-
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
export: Allow --node-data
to be repeated to build up a file list
#1010
Conversation
ISTR discussing this behaviour (or similar) before but can't recall specifics. In any case, I think the behaviour in this PR is more favorable, but it's definitely up for discussion. Are there many (any?) cases where we really do want the overriding behaviour? |
Codecov ReportBase: 61.68% // Head: 59.68% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #1010 +/- ##
==========================================
- Coverage 61.68% 59.68% -2.00%
==========================================
Files 52 53 +1
Lines 6287 6323 +36
Branches 1583 1587 +4
==========================================
- Hits 3878 3774 -104
- Misses 2138 2286 +148
+ Partials 271 263 -8
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View 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.
This seems good to go from my perspective. It's not something I've come across but I agree that the interpretation of repeated --node-data
should be the union of files, not simply the last one(s), and thus think it should be merged.
47f546f
to
f48f196
Compare
…ort v2 overwrite each other
…instead of each repetition overriding the last values. While there is some theoretical utility in being able to override one list of node data files with another in a subsequent option, I think it's more likely that repetition is intended to built up a single list. I was surprised, at least. See also <https://discussion.nextstrain.org/t/1194/4>.
f48f196
to
de8147b
Compare
Updated the changelog in f48f196 and then rebased onto latest master to resolve conflicts before merge. |
When resolving conflicts prior to merging¹, I didn't notice that the new entry I was adding was now under an already-released version. ¹ #1010
Description of proposed changes
…instead of each repetition overriding the last values. While there is some theoretical utility in being able to override one list of node data files with another in a subsequent option, I think it's more likely that repetition is intended to built up a single list. I was surprised, at least.
See also https://discussion.nextstrain.org/t/1194/4.
Testing
Checklist