-
Notifications
You must be signed in to change notification settings - Fork 128
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
fix: move custom extensions to meta in export v2 #888
Conversation
Codecov Report
@@ Coverage Diff @@
## master #888 +/- ##
=======================================
Coverage 34.24% 34.24%
=======================================
Files 41 41
Lines 5986 5986
Branches 1535 1535
=======================================
Hits 2050 2050
Misses 3854 3854
Partials 82 82
Continue to review full report at Codecov.
|
@@ -1037,7 +1037,7 @@ def run_v2(args): | |||
|
|||
# pass through any extensions block in the auspice config JSON without any changes / checking | |||
if config.get("extensions"): | |||
data_json["extensions"] = config["extensions"] | |||
data_json["meta"]["extensions"] = config["extensions"] |
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.
Technically this is a breaking change which would prompt a major version bump, but if we're very sure that no one else is using this, maybe OK to be a bug fix instead?
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.
True, I don't mind either way - 14.1.0 was released around a week ago?
Up to you :)
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.
It's also no biggie to bump to version 15 and not have to hem and haw over if it's ok to break our rules this time or not. :-) Version numbers are cheap!
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.
Looks good to me - I would be extremely surprised if anyone has used this key!
resolves #885
I misspecified where extensions should be placed by augur. I hadn’t made it clear that Nextclade has settled on
meta.extensions
and I didn’t notice it when reviewing the PR #865. So now Augur places extensions at top level - which Nextclade doesn’t accept.This PR moves extensions to where Nextclade expects it.
Only a few code changes were required. The tests pass.