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

BF: do not add [Default: ...] for --help if it is a flag (action="store_...") #4002

Merged
merged 2 commits into from Jan 12, 2020

Conversation

yarikoptic
Copy link
Member

@yarikoptic yarikoptic commented Jan 9, 2020

Closes #3203

Or there could be a flag without action="store_...", or some store_ wouldn't be a flag?

I found no other conveniently available unittesting setup to test this change. Any pointers or assistance would be appreciated

yarikoptic added 2 commits Jan 9, 2020
"is None a string which is considered by default?"  would be a logical
question of any cmdline user who is not aware that DataLad is written in
Python
@yarikoptic
Copy link
Member Author

@yarikoptic yarikoptic commented Jan 9, 2020

while at it expanded cleansing in eed4dde

ENH: also do not include misguiding [Default: None]

"is None a string which is considered by default?"  would be a logical
question of any cmdline user who is not aware that DataLad is written in
Python

@codecov
Copy link

@codecov codecov bot commented Jan 9, 2020

Codecov Report

Merging #4002 into 0.11.x will increase coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           0.11.x    #4002      +/-   ##
==========================================
+ Coverage   89.92%   89.96%   +0.04%     
==========================================
  Files         253      253              
  Lines       34078    34080       +2     
==========================================
+ Hits        30643    30661      +18     
+ Misses       3435     3419      -16
Impacted Files Coverage Δ
datalad/interface/base.py 90.09% <100%> (+0.06%) ⬆️
datalad/downloaders/tests/test_http.py 91.15% <0%> (+2.21%) ⬆️
datalad/downloaders/http.py 87.3% <0%> (+2.77%) ⬆️

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 4776c27...eed4dde. Read the comment docs.

# showing the Default: (likely boolean).
# See https://github.com/datalad/datalad/issues/3203
if not parser_kwargs.get('action', '').startswith('store_'):
# [Default: None] also makes little sense for cmdline
Copy link
Member

@bpoldrack bpoldrack Jan 10, 2020

Choose a reason for hiding this comment

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

The argument for [Default: None] being confusing for command line holds true for non-flag options as well or am I missing something?

Copy link
Member

@bpoldrack bpoldrack Jan 10, 2020

Choose a reason for hiding this comment

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

BTW: Same argument is true for True and False in general. It's not (necessarily) referring to those strings.

Copy link
Member Author

@yarikoptic yarikoptic Jan 10, 2020

Choose a reason for hiding this comment

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

hm, ... what if there is a choice allowing for bool and some strings, and default is a bool? Note: This block is executed only for non store_, so all flags are excluded already by here, but IMHO it shouldn't be generalized solely based on a default value

Copy link
Member Author

@yarikoptic yarikoptic Jan 10, 2020

Choose a reason for hiding this comment

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

but now I am getting a better clue on what you meant ;) indeed it is unlikely we have bool specified as a value (e.g. as True) in command line... we do though have assure_bool etc handling which was/is used by crawler, but I am not sure we have anything like that for command line options. I would though probably not dive into deep analysis at this point -- could be improved later if we run into such odd places.

kyleam
kyleam approved these changes Jan 10, 2020
Copy link
Contributor

@kyleam kyleam left a comment

I would though probably not dive into deep analysis at this point -- could be improved later if we run into such odd places.

From my POV that's fine. This PR already leads to a very nice improvement in the presentation. Just one example, comparing siblings:

siblings --help diff
--- siblings.before
+++ siblings.after
@@ -60,40 +60,36 @@
                         given, an attempt is made to identify the dataset
                         based on the input and/or the current working
                         directory. Constraints: Value must be a Dataset or a
-                        valid identifier of a Dataset (e.g. a path) [Default:
-                        None]
+                        valid identifier of a Dataset (e.g. a path)
   -s NAME, --name NAME  name of the sibling. For addition with path "URLs" and
                         sibling removal this option is mandatory, otherwise
                         the hostname part of a given URL is used as a default.
                         This option can be used to limit 'query' to a specific
-                        sibling. Constraints: value must be a string [Default:
-                        None]
+                        sibling. Constraints: value must be a string
   --url [URL]           the URL of or path to the dataset sibling named by
                         NAME. For recursive operation it is required that a
                         template string for building subdataset sibling URLs
                         is given. List of currently available placeholders:
                         %NAME the name of the dataset, where slashes are
                         replaced by dashes. Constraints: value must be a
-                        string [Default: None]
+                        string
   --pushurl PUSHURL     in case the URL cannot be used to publish to the
                         dataset sibling, this option specifies a URL to be
                         used instead. If no URL is given, PUSHURL serves as
                         URL as well. Constraints: value must be a string
-                        [Default: None]
   -D DESCRIPTION, --description DESCRIPTION
                         short description to use for a dataset location. Its
                         primary purpose is to help humans to identify a
                         dataset copy (e.g., "mike's dataset on lab server").
                         Note that when a dataset is published, this
                         information becomes available on the remote side.
-                        Constraints: value must be a string [Default: None]
-  --fetch               fetch the sibling after configuration. [Default:
-                        False]
+                        Constraints: value must be a string
+  --fetch               fetch the sibling after configuration.
   --as-common-datasrc NAME
                         configure the created sibling as a common data source
                         of the dataset that can be automatically used by all
                         consumers of the dataset (technical: git-annex auto-
-                        enabled special remote). [Default: None]
+                        enabled special remote).
   --publish-depends SIBLINGNAME
                         add a dependency such that the given existing sibling
                         is always published prior to the new sibling. This
@@ -101,42 +97,38 @@
                         'remote.SIBLINGNAME.datalad-publish-depends'. This
                         option can be given more than once to configure
                         multiple dependencies. Constraints: value must be a
-                        string [Default: None]
+                        string
   --publish-by-default REFSPEC
                         add a refspec to be published to this sibling by
                         default if nothing specified. Constraints: value must
-                        be a string [Default: None]
+                        be a string
   --annex-wanted EXPR   expression to specify 'wanted' content for the
                         repository/sibling. See https://git-
                         annex.branchable.com/git-annex-wanted/ for more
                         information. Constraints: value must be a string
-                        [Default: None]
   --annex-required EXPR
                         expression to specify 'required' content for the
                         repository/sibling. See https://git-
                         annex.branchable.com/git-annex-required/ for more
                         information. Constraints: value must be a string
-                        [Default: None]
   --annex-group EXPR    expression to specify a group for the repository. See
                         https://git-annex.branchable.com/git-annex-group/ for
                         more information. Constraints: value must be a string
-                        [Default: None]
   --annex-groupwanted EXPR
                         expression for the groupwanted. Makes sense only if
                         --annex-wanted="groupwanted" and annex-group is given
                         too. See https://git-annex.branchable.com/git-annex-
                         groupwanted/ for more information. Constraints: value
-                        must be a string [Default: None]
+                        must be a string
   --inherit             if sibling is missing, inherit settings (git config,
                         git annex wanted/group/groupwanted) from its super-
-                        dataset. [Default: False]
+                        dataset.
   --no-annex-info       Whether to query all information about the annex
                         configurations of siblings. Can be disabled if speed
-                        is a concern. [Default: True]
-  -r, --recursive       if set, recurse into potential subdataset. [Default:
-                        False]
+                        is a concern.
+  -r, --recursive       if set, recurse into potential subdataset.
   -R LEVELS, --recursion-limit LEVELS
                         limit recursion into subdataset to the given number of
                         levels. Constraints: value must be convertible to type
-                        'int' [Default: None]
+                        'int'
 

meta-nit:

Closes #3203

This won't actually close the issue because this is based on 0.11.x rather than master. In this case, it needs to be in the commit message (where I think it should be, anyway).

@yarikoptic
Copy link
Member Author

@yarikoptic yarikoptic commented Jan 11, 2020

Good point Kyle. I by default left fixes statements to pr. Should change my habits.
If no further comments/objections, let's merge?

@yarikoptic
Copy link
Member Author

@yarikoptic yarikoptic commented Jan 12, 2020

ok, no objections were expressed, so let's do it

@yarikoptic yarikoptic merged commit 0745a5c into datalad:0.11.x Jan 12, 2020
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants