-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add unit test for ensembl.io.genomio.genome_metadata.prepare module #312
Conversation
also update NCBI's provider URLS
…and add unit tests
assembly["version"] = int(values[1]) | ||
version = accession.partition(".")[2] | ||
if version: | ||
assembly["version"] = int(version) | ||
|
||
|
||
def add_genebuild_metadata(genome_data: Dict) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps use a bit more descriptive function name here. Perhaps: add_genebuild_version_metadata
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is adding both version
and start_date
, so I think in this case the function name is appropriate. That being said, maybe referring to annotation rather than genebuild would be more accurate, but that can be discussed separaterly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor things only, happy to approve as is. Up to you on comment suggestions
if (not "provider_name" in assembly) and (not "provider_url" in assembly): | ||
assembly["provider_name"] = provider["assembly"]["provider_name"] | ||
assembly["provider_url"] = provider["assembly"]["provider_url"] | ||
assembly["provider_url"] = f'{provider["assembly"]["provider_url"]}/{accession}' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure we want the accession here: It's the URL for the provider, not for the data (and there is no guarantee the provider_url can have /accession be valid outside of Refseq)
Updated all methods (review docstrings, improve readability and reduce code complexity) and added unit tests for all of them.