Skip to content
This repository was archived by the owner on Jun 15, 2024. It is now read-only.

Conversation

@sabSaravanan
Copy link
Collaborator

No description provided.

@GreenCappuccino GreenCappuccino self-requested a review February 25, 2023 00:29
Copy link
Member

@GreenCappuccino GreenCappuccino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also fixup all linter issues as shown on this PR. I recommend setting up the pre-commit with poetry install and poetry run task pre-commit.

More useful documentation should document a functional overview of code, and not what specific lines of code do, which should be already relatively obvious.

meili = request.state.meili
loop = asyncio.get_running_loop()

"""initializes database, miniothe database connection"""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Multi-line strings are generally only used in docstrings. Add docstrings under class/function/method headers, or else they won't be parsed. See https://peps.python.org/pep-0257/

Otherwise all other internal comments should be normal octothorpe comments.

Also, please fix typos.

content = await file.read()

with ThreadPoolExecutor() as pool:
"""function uses ThreadPoolExecutor to parse the SDS file into JSON using the SigmaAldrichSdsParse."""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above. Context managers are not functions.

)
)

"""Uploads the file to Minio and creates a URL for the file"""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above, multi-line strings discouraged compared to octothorpe comments.

db = request.state.db

async with db.begin():
"""retrieves records from the SafetyDataSheet where the `id` field matches any value in the `sds_ids` ."""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Context managers not definitions.

stmt = select(SafetyDataSheet).where(SafetyDataSheet.id == func.any(sds_ids))
result = await db.execute(stmt)

"""returns the batch of SDS sheets"""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

etc. etc.

@GreenCappuccino GreenCappuccino marked this pull request as draft April 13, 2023 19:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants