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 factory method on Application for customizing the Loader parsing cmd-line args #360

Merged
merged 5 commits into from Jan 13, 2017

Conversation

ankostis
Copy link
Contributor

@ankostis ankostis commented Jan 11, 2017

Add capability to modify the the Loader used to parse cmd-lines used by Application class (currently the KVArgParseConfigLoader class) without fully reimplementing Application.parse_command_line() method.

Rationale

With the above customization it is easier to modify the argparse parser, which has been enabled by #322.
A case where the above is needed arrives in projects with multiple sub-cmds, where some cmd-names clash, and argparse "conflicts" occur (when adding the same option, twice).

While the argparse constructor offers the conflict_handler='resolve' strategy,and there exists an argparse factory method on the Loder, there is no equivalent factory method on the Application for the Loader itself - that means that you cannot modify the former factory by switching Loader implementations, unless you re-implement Application.parse_command_line(), which is not trivial code.

An alternative to this PR would be to add all *argparse_args and **argparse_kwds onApplication class, and pass them when constructing the Loader, which should then pass them to argparse, but a) this violates blatantly the Demeter law adding dead weight for most cases, and b) being easier to override the Loader might be useful for other cases as well.

- Assume a user viewpoint when explaining how configurables work.
- Use `====` and `---` tiele headers to denote hierarchical sections in
the help message.
- Don't add blanklines below section-headers, `===` should be enough -
also this makes the separation with the next section more visible.
@ankostis
Copy link
Contributor Author

(Just based on top of #354, I can rebase it on master if asked)

@minrk minrk added this to the 5.0 milestone Jan 13, 2017
@minrk minrk merged commit 4708d31 into ipython:master Jan 13, 2017
@minrk
Copy link
Member

minrk commented Jan 13, 2017

Makes sense, thanks

@ankostis ankostis deleted the loaderfact branch January 13, 2017 18:43
@ankostis ankostis mentioned this pull request Aug 21, 2017
2 tasks
ankostis added a commit to ankostis/traitlets that referenced this pull request Feb 4, 2018
Similar to ipython#360, trying introduce a mew config-loader (e.g. YAML or
salt) without this commmit would require to re-implent
`Application._load_config_files()`.
@Carreau Carreau added 5.0-re-review Need to re-review for potential API impact changes. 5.0-minor rereviewed, minor change need to be put in changelog. labels Jun 4, 2020
@Carreau Carreau removed the 5.0-re-review Need to re-review for potential API impact changes. label Jun 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5.0-minor rereviewed, minor change need to be put in changelog.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants