Adding support to the context to distinguish the source of a command line parameter #1329
Conversation
|
Could you add an entry to the changelog for this, and it might be good to add a note to the docs about the change. |
|
What section would you recommend that I add the docs to? |
|
Good question, I think it makes sense to add a section to https://click.palletsprojects.com/en/7.x/parameters/ since your change applies to everything that inherits from BaseCommand. |
|
Maybe it makes more sense to put it in the advanced.rst file? The parameters.rst file is pretty small and the concepts around contexts, environment variables and default values are in the options.rst, arguments.rst, and commands.rst files, which come after the parameters.rst file in the documentation. |
|
That sounds good to me, it is a little to specific to go into the parameters section. |
…_MAP as a source and added unit tests for it.
|
@jcrotts, docs have been added and tests are passing. Please let me know if there are any other changes you want. |
jab
left a comment
jab
left a comment
There was a problem hiding this comment.
Awesome @joshuastorck, I've been wanting this – thanks for submitting! A few suggestions – what do you think? Psyched to get this landed!
|
|
||
| return sorted(declaration_order, key=sort_key) | ||
|
|
||
| class ParameterSource(object): |
There was a problem hiding this comment.
nit: Should this be named OptionSource? Positional arguments can't be set via the environment, can't have default values, etc. so this is only relevant for options. I think this was the intention since the docstring says "indicates the source of a command line option" already. https://click.palletsprojects.com/en/7.x/parameters/#differences
There was a problem hiding this comment.
https://click.palletsprojects.com/en/7.x/arguments/#environment-variables states that arguments can be set via the environment, will change the docs to parameter from option.
There was a problem hiding this comment.
@joshuastorck Sounds like the docs are incorrect then? https://click.palletsprojects.com/en/7.x/parameters/#differences says "The following features are only available for options: ... option values can be pulled from environment variables, arguments can not". If so then I agree it makes sense to leave this named ParameterSource but I wonder if it's worth at least a comment mentioning that the source of an argument can never be e.g. DEFAULT, if not actually asserting that in the validate logic?
…tead of 'option'. Removed emphasis of the word source in Context.get_parameter_source. Changed ParameterSource.VALUE to a set instead of list. Fixed bugs in ParameterSource.validate and added unit test for it. Changed Context.get_parameter_source to return a KeyError when an invalid parameter name is specified.
|
@jab, I think I've addressed all of your comments. Please take another look. |
jab
left a comment
jab
left a comment
There was a problem hiding this comment.
Thanks @joshuastorck, looking great, just a couple last things and then I think this should be ready to merge!
|
|
||
| return sorted(declaration_order, key=sort_key) | ||
|
|
||
| class ParameterSource(object): |
There was a problem hiding this comment.
@joshuastorck Sounds like the docs are incorrect then? https://click.palletsprojects.com/en/7.x/parameters/#differences says "The following features are only available for options: ... option values can be pulled from environment variables, arguments can not". If so then I agree it makes sense to leave this named ParameterSource but I wonder if it's worth at least a comment mentioning that the source of an argument can never be e.g. DEFAULT, if not actually asserting that in the validate logic?
… value. Changing Context._sources_by_paramname to Context._source_by_paramname. Renaming ParameterSource.validate clz arg to cls and removing doc for that argument in the docstring
|
Looks good, thanks! |
4 participants
Fixes #1264