Skip to content
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

Merged
merged 18 commits into from
Mar 19, 2024

Conversation

JAlvarezJarreta
Copy link
Contributor

Updated all methods (review docstrings, improve readability and reduce code complexity) and added unit tests for all of them.

assembly["version"] = int(values[1])
version = accession.partition(".")[2]
if version:
assembly["version"] = int(version)


def add_genebuild_metadata(genome_data: Dict) -> None:
Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Member

@ens-LCampbell ens-LCampbell left a 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}'
Copy link
Contributor

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)

@MatBarba MatBarba merged commit b25b99d into hackathon/feb24 Mar 19, 2024
1 check passed
@MatBarba MatBarba deleted the jalvarez/hack_feb24_r3 branch March 19, 2024 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants