DM-35917: Migrate config setting docs from pipe_base and update code to match expectations #199
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
timj
commented
Aug 20, 2022
| show = ShowInfo(kwargs.pop("show", [])) | ||
| pipeline = script.build(**kwargs, show=show) | ||
| if show.handled and not show.unhandled: | ||
| # The show option was given and all options were processed. |
There was a problem hiding this comment.
I wonder if we should print this to the user so they aren't confused.
There was a problem hiding this comment.
Hmm, if the output from e.g. dump-config is redirected to a file, it may also confuse something.
There was a problem hiding this comment.
stderr is probably better than stdout, though we should probably ask users about what they want. I myself almost never use pipetask, so my opinion probably should not count.
timj
force-pushed
the
tickets/DM-35917
branch
2 times, most recently
from
7619b97
to
d790409
Compare
Codecov Report
@@ Coverage Diff @@
## main #199 +/- ##
==========================================
+ Coverage 82.42% 83.35% +0.93%
==========================================
Files 47 48 +1
Lines 3755 3846 +91
Branches 676 690 +14
==========================================
+ Hits 3095 3206 +111
+ Misses 481 470 -11
+ Partials 179 170 -9
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
timj
force-pushed
the
tickets/DM-35917
branch
2 times, most recently
from
c635871
to
8100e02
Compare
timj
changed the title
1. Keep track of which options have been used. 2. No longer run the pipeline show options multiple times for pipetask qgraph. 3. No longer warn about options not being used when they are about to be used. 4. If --show is used, no longer continue on unless there are remaining --show options to process. The latter means that using `pipetask qgraph` with only pipeline show options exits without building the graph, and any use of --show leads to `pipetask run` not running anything.
This matches what the click option callback is doing itself and so ensures consitency in option handling.
timj
force-pushed
the
tickets/DM-35917
branch
3 times, most recently
from
1f8a19c
to
abd210a
Compare
Also added some additions.
This is easier to test and it is odd having a class be able to exit.
This makes it easier to test the output.
2 tasks
andy-slac
approved these changes
Aug 24, 2022
There is a non-zero chance that people might want to specify a command-line config option that includes a comma and it's safer to require people to use multiple --config options.
This makes it clear why the command finished without doing anything.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Whilst migrating some config docs from pipe_base I realized that the --show option was not working right. I ended up trying to fix it.
Checklist
doc/changes