-
Notifications
You must be signed in to change notification settings - Fork 129
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
merge: Omit generated source columns by default #1632
Conversation
Removes default naming template and requires users to explicitly provide their own template to include source columns. This makes the output from an `augur merge` invocation more self-documenting without columns "magically" appearing. In the expected context of usage within a workflow, the burden of the extra option is negligible. See also discussion on a prior PR.¹ ¹ <#1625 (comment)>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1632 +/- ##
=======================================
Coverage 71.06% 71.06%
=======================================
Files 79 79
Lines 8268 8268
Branches 2010 2010
=======================================
Hits 5876 5876
Misses 2101 2101
Partials 291 291 ☔ View full report in Codecov by Sentry. |
Once this is merged, I'd like to cut a new (major) Augur release and get it published everywhere so I can then merge nextstrain/measles#52. |
output_group.add_argument('--no-source-columns', dest="source_columns", action="store_const", const=None, help=f"Suppress generated columns (described above) identifying the source of each row's data. This is the default behaviour, but it may be made explicit or used to override a previous --source-columns." + SKIP_AUTO_DEFAULT_IN_HELP) | ||
output_group.add_argument('--quiet', action="store_true", default=False, help="Suppress informational and warning messages normally written to stderr. (default: disabled)" + SKIP_AUTO_DEFAULT_IN_HELP) |
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.
Why keep --no-source-columns
around when it is already the default behavior? It seems unnecessary, with the equivalent being options such as --not-quiet
or --no-output-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.
It's a bit arbitrary/preference, but in general --x
/ --no-x
is a nice (and very common) pattern to allow disabling previously-set options. This allows more flexibility in command invocation construction (e.g. a fixed list of options including --x
can be changed by tacking on --no-x
instead of finding and removing --x
). It also provides a way for invocations to opt-into usages which aren't broken by changes to the defaults. We haven't generally done this as a matter of course in Augur, but I don't think it'd be a bad idea to start and it was easy to keep this pattern here.
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.
See also click's implementation of this pattern.
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.
Thanks for the explanation, makes sense!
If you end up doing this while I'm gone on lab retreat, could you also merge other breaking changes #1613 and #1629? Those have been approved and I just haven't gotten around to doing the actual release. The only thing is there will be changelog merge conflicts. |
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, I'll defer to you for #1632 (comment) so consider that non-blocking.
Removes default naming template and requires users to explicitly provide their own template to include source columns. This makes the output from an
augur merge
invocation more self-documenting without columns "magically" appearing. In the expected context of usage within a workflow, the burden of the extra option is negligible. See also discussion on a prior PR.¹¹ #1625 (comment)
Checklist