Skip to content
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

add --on-failure default value and document it #5690

Merged
merged 2 commits into from Jun 14, 2021

Conversation

christian-monch
Copy link
Contributor

Set and document default value "continue" for common argument --on-failure, and "default" for common argument --output-format

There was a comment that misleadingly stated for --on-failure: "no default: better be configure per-command". If this was true, we could not set or document a general default-value for --on-failure. I did not find per-command configurations, therefore I added the default value to the doc-string and as keyword argument

Fixes #4334

@codecov
Copy link

codecov bot commented May 21, 2021

Codecov Report

Merging #5690 (53e910e) into maint (0c6d355) will decrease coverage by 5.85%.
The diff coverage is 93.75%.

❗ Current head 53e910e differs from pull request most recent head 114b46e. Consider uploading reports for the commit 114b46e to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##            maint    #5690      +/-   ##
==========================================
- Coverage   90.32%   84.46%   -5.86%     
==========================================
  Files         299      296       -3     
  Lines       42349    42318      -31     
==========================================
- Hits        38251    35744    -2507     
- Misses       4098     6574    +2476     
Impacted Files Coverage Δ
datalad/core/distributed/clone.py 90.10% <ø> (-1.53%) ⬇️
datalad/core/distributed/push.py 88.52% <ø> (ø)
datalad/core/local/create.py 92.36% <ø> (-0.70%) ⬇️
datalad/core/local/diff.py 90.62% <ø> (ø)
datalad/core/local/run.py 98.23% <ø> (-0.89%) ⬇️
datalad/core/local/status.py 96.42% <ø> (ø)
datalad/distributed/create_sibling_ria.py 82.35% <ø> (ø)
datalad/distributed/export_archive_ora.py 81.53% <ø> (ø)
datalad/distribution/create_sibling.py 82.81% <ø> (-3.10%) ⬇️
datalad/distribution/create_sibling_github.py 84.37% <ø> (ø)
... and 141 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0c6d355...114b46e. Read the comment docs.

Copy link
Member

@mih mih left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like we should have ways to not have to duplicate a default value between default= and the doc string. However, I do not feel enough motivation to figure it out in general as a precondition to get this specific case resolved. Happy to see it merged, if other agree.

Copy link
Contributor

@kyleam kyleam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't re-familiarized myself with how this is all wired up, but overall this PR looks like an improvement to me. Here's a diff of the datalad --help output.

diff --git a/help-output-0c6d355af98f0e1dfccd96c84cc77c5ab7a9121d b/help-output-02fa5192d99e0c49e9c23c87d1546fd82015066b
index c226aa258e..dc13507ec7 100644
--- a/help-output-0c6d355af98f0e1dfccd96c84cc77c5ab7a9121d
+++ b/help-output-02fa5192d99e0c49e9c23c87d1546fd82015066b
@@ -160,7 +160,7 @@ specific --help, i.e.: datalad <command> --help
                         '{metadata[name]}'. If a 2nd-level key contains a
                         colon, e.g. 'music:Genre', ':' must be substituted by
                         '#' in the template, like so:
-                        '{metadata[music#Genre]}'.
+                        '{metadata[music#Genre]}'. [Default: 'default']
   --report-status {success,failure,ok,notneeded,impossible,error}
                         constrain command result report to records matching
                         the given status. 'success' is a synonym for 'ok' OR
@@ -177,7 +177,8 @@ specific --help, i.e.: datalad <command> --help
                         'continue' works like 'ignore', but an error causes a
                         non-zero exit code; 'stop' halts on first failure and
                         yields non-zero exit code. A failure is any result
-                        with status 'impossible' or 'error'.
+                        with status 'impossible' or 'error'. [Default:
+                        'continue']
   --cmd                 syntactical helper that can be used to end the list of
                         global command line options before the subcommand
                         label. Options taking an arbitrary number of arguments

@@ -324,6 +329,7 @@
constraints=EnsureChoice(*list(known_result_xfms.keys())) | EnsureCallable() | EnsureNone()),
result_renderer=Parameter(
doc="""format of return value rendering on stdout""",
default='default',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell, this particular line is unnecessary (see script below). Adding it seems likely to confuse readers given that these common_opts parameters apply to both the Python and command-line interface, but the default result_renderer value for Python callers is None.

script
set -eu

base=0c6d355af98f0e1dfccd96c84cc77c5ab7a9121d
pr=02fa5192d99e0c49e9c23c87d1546fd82015066b

doc_dump () {
    commit="$1"
    git checkout --quiet "$commit"
    python -m pydoc datalad.api.push >pydoc-output-"$commit" 2>&1
    datalad --help >help-output-"$commit" 2>&1
}

doc_diff () {
    reva="$1"
    revb="$2"
    git diff --no-index pydoc-output-"$reva" pydoc-output-"$revb" || :
    git diff --no-index help-output-"$reva" help-output-"$revb" || :

}

doc_dump "$base"
doc_dump "$pr"
doc_diff "$base" "$pr"

pr_revised=9ae17ce4fc44447b005ce87fcc53f1280aef1572
git diff "$pr" "$pr_revised"

echo "No changes:"
doc_dump "$pr_revised"
doc_diff "$pr" "$pr_revised"
diff --git a/help-output-0c6d355af98f0e1dfccd96c84cc77c5ab7a9121d b/help-output-02fa5192d99e0c49e9c23c87d1546fd82015066b
index c226aa258e..dc13507ec7 100644
--- a/help-output-0c6d355af98f0e1dfccd96c84cc77c5ab7a9121d
+++ b/help-output-02fa5192d99e0c49e9c23c87d1546fd82015066b
@@ -160,7 +160,7 @@ specific --help, i.e.: datalad <command> --help
                         '{metadata[name]}'. If a 2nd-level key contains a
                         colon, e.g. 'music:Genre', ':' must be substituted by
                         '#' in the template, like so:
-                        '{metadata[music#Genre]}'.
+                        '{metadata[music#Genre]}'. [Default: 'default']
   --report-status {success,failure,ok,notneeded,impossible,error}
                         constrain command result report to records matching
                         the given status. 'success' is a synonym for 'ok' OR
@@ -177,7 +177,8 @@ specific --help, i.e.: datalad <command> --help
                         'continue' works like 'ignore', but an error causes a
                         non-zero exit code; 'stop' halts on first failure and
                         yields non-zero exit code. A failure is any result
-                        with status 'impossible' or 'error'.
+                        with status 'impossible' or 'error'. [Default:
+                        'continue']
   --cmd                 syntactical helper that can be used to end the list of
                         global command line options before the subcommand
                         label. Options taking an arbitrary number of arguments
diff --git a/datalad/interface/common_opts.py b/datalad/interface/common_opts.py
index 6246085674..fdba2e90e3 100644
--- a/datalad/interface/common_opts.py
+++ b/datalad/interface/common_opts.py
@@ -329,7 +329,6 @@
         constraints=EnsureChoice(*list(known_result_xfms.keys())) | EnsureCallable() | EnsureNone()),
     result_renderer=Parameter(
         doc="""format of return value rendering on stdout""",
-        default='default',
         constraints=EnsureChoice('default', 'json', 'json_pp', 'tailored') | EnsureNone()),
     on_failure=Parameter(
         doc="""behavior to perform on failure: 'ignore' any failure is reported,
No changes:

@yarikoptic yarikoptic added the semver-patch Increment the patch version when merged label Jun 2, 2021
Relation/difference in CLI invocation defaults is now explicitly
annotated.

Centralization:

- We can discover defaults via a simple loop, no need to move
  defaults specification to eval_defaults.

  This also helps to avoid coming up with per-param variables
  to contain the defaults

- argparser provides  default value via string interpolation, so
  we can just use %(default)s and %(default)r if so desired, and
  avoid referring to variables etc. Simplifies and harmonizes.

- a comment added about intended (IMHO) difference for result_renderer
  defaults.

also minimized diff in empty lines etc.

Note:

I thought that if no default is provided to parser's argument,
default would be the first among choices.  I have not looked in detail,
but how we use argparser - it is not the case.  If not specified, we are
ending up with None for --on-failure, and thus simply not passing it down
to __call__ and thus getting back to the eval_defaults values.

So, it should be ok to explicitly specify the default (note: order of choices
does not have default first).

RF/BF: result-renderer - revert back to "default" in CLI and None in Python
@yarikoptic
Copy link
Member

Thanks @christian-monch for striving to reduce duplication, centralize definitions and make users more aware of what-the-heck is the defaults ;-) I could not express myself in a small suggestion diff, so pushed a commit straight into the branch -- it has detailed description which hopefully makes intentions etc clearer.

Here is my version of Kyle's handy script
# from https://github.com/datalad/datalad/pull/5690#discussion_r638931994
# with coming back to original commit

set -eu

# from maint to CM's version
#base=0c6d355af98f0e1dfccd96c84cc77c5ab7a9121d
#pr=02fa5192d99e0c49e9c23c87d1546fd82015066b

# from CM's to Yarik's version
#base=02fa5192d99e0c49e9c23c87d1546fd82015066b
#pr=8bacaf2f9acb14f0d3c8ca38a96285a75b0226d6

base="$(git rev-parse $1)"
pr="$(git rev-parse $2)"

echo "Comparing $1 ($base) and $2 ($pr)"

doc_dump () {
    commit="$1"
    git checkout --quiet "$commit"
    python -m pydoc datalad.api.push >pydoc-output-"$commit" 2>&1
    datalad --help >help-output-"$commit" 2>&1
    git checkout --quiet -
}

doc_diff () {
    reva="$1"
    revb="$2"
    git diff -U18 --no-index pydoc-output-"$reva" pydoc-output-"$revb" || :
    git diff -U18 --no-index help-output-"$reva" help-output-"$revb" || :

}

doc_dump "$base"
doc_dump "$pr"
doc_diff "$base" "$pr"

echo "done"

which if I run on diff from your state and my commit -- there is no diff

$> ../trash/gh-5690-kyleam.sh 02fa5192d99e0c49e9c23c87d1546fd82015066b HEAD   
Comparing 02fa5192d99e0c49e9c23c87d1546fd82015066b (02fa5192d99e0c49e9c23c87d1546fd82015066b) and HEAD (114b46e596befecec3c9d8eb9f3128d5f1a9b571)
done

so my changes did not effect the output, but I think they localized defaults better with more explicit overloading of the default value for result-renderer in CLI as opposed to the one in eval_defaults.

Copy link
Member

@mih mih left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thx all!

@mih
Copy link
Member

mih commented Jun 14, 2021

Restarted missing test

@mih
Copy link
Member

mih commented Jun 14, 2021

Passed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch Increment the patch version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Common cmdline options, like --on-failure, do not state defaults
4 participants