-
Notifications
You must be signed in to change notification settings - Fork 130
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
feat: --skip-validation allows mismatched Augur versions #902
Conversation
Codecov Report
@@ Coverage Diff @@
## master #902 +/- ##
==========================================
+ Coverage 59.64% 59.67% +0.02%
==========================================
Files 53 53
Lines 6317 6319 +2
Branches 1586 1586
==========================================
+ Hits 3768 3771 +3
+ Misses 2286 2285 -1
Partials 263 263
Continue to review full report at Codecov.
|
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.
Thank you, @corneliusroemer! I had a couple minor comments about the tests, but otherwise good to merge. (Thank you for adding tests!)
@@ -9,8 +9,9 @@ | |||
|
|||
|
|||
class NodeDataFile: | |||
def __init__(self, fname): | |||
def __init__(self, fname, skip_validation=False): |
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.
I agree it's annoying to pass around this variable through multiple classes. This isn't ideal OO design and an OO implementation is even what we need here. Your implementation here seems fine, in the current context. We should leave to another PR to reimplement the node data reader as a function again. 😄
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.
Yes!
As suggested by @huddlej in PR #902 Tests originally implemented by @corneliusroemer
6cff6e7
to
404595d
Compare
I really needed this today so I took it upon myself to implement John's PR suggestions & also rebased it onto current master (99 commits ahead!). I think this should be merged 🙏 |
Test failure is the same as seen on another PR: #961 (comment) and seems unrelated to this PR 😕 |
As suggested by @huddlej in PR #902 Tests originally implemented by @corneliusroemer
404595d
to
c74c2ab
Compare
This was bumped in Slack. Force-pushed a rebase on latest |
2be8e8c
to
61f2ae0
Compare
Description of proposed changes
Currently, files produced by Augur contain the version of the Augur instance used to produce the data file.
When there's a major version mismatch between the version a file was produced with and the version of Augur
export
, Augur throws an error complaining about version mismatch.While it's a good thing to warn the user, there are situations where one may want to take the risk and allow Augur export to use output from different versions.
This PR extends the meaning of the
--skip-validation
argument to also not complain about version mismatch in input files.--skip-validation
was introduced in #865Related issue(s)
Fixes #901
Related to #865
Testing
I added integration tests