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.

Sign up for GitHub

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

Jump to bottom

add tb tests #64

Merged
merged 11 commits into from
Oct 11, 2021
Merged

add tb tests #64

merged 11 commits into from
Oct 11, 2021

Conversation

jodyphelan
Copy link
Contributor

No description provided.

@jodyphelan
Copy link
Contributor Author

Sorry for all the commits, still trying to figure out the different connected parts

@fmaguire
Copy link
Member

No worries, let me know if anything is unclear (or if the documentation needs to be better). The more complex inheritance based code architecture was to try and make adding new parsers easier and reduce code duplication but it doesn't always achieve that goal!

@fmaguire
Copy link
Member

If you cd into the test directory and run pytest and run_test.sh it will execute the same tests locally that the CI is running.

@cimendes
Copy link
Member

You don't need to cd into test. pytest is smart and will figure it out :D

@jodyphelan
Copy link
Contributor Author

Ah thanks!

jodyphelan
jodyphelan commented Oct 11, 2021
View reviewed changes
hAMRonization/hAMRonizedResult.py

# variant specific optional fields
variant_frequency: float = None
genetic_variation_type: str = None
Copy link
Contributor Author

Choose a reason for hiding this comment

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

At the moment I've made this non mandatory but potentially in the final spec it should be?

Copy link
Member

Choose a reason for hiding this comment

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

This will be lost when I approve and merge. Can you open an issue? ❤️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure!

@jodyphelan
Copy link
Contributor Author

jodyphelan commented Oct 11, 2021 •
edited
Loading

Yay we got there in the end! Apologies again for all the tests

@cimendes cimendes merged commit ee85f6d into pha4ge:master Oct 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cimendes cimendes cimendes approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jodyphelan @fmaguire @cimendes