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

Ability to Specify Global Arguments, Reduce Redunancy for Common Argument #63

Closed
tychoish opened this issue May 18, 2014 · 16 comments
Closed

Comments

@tychoish
Copy link

This may be documentation bugs, and it might be the case that a few simple helper functions/decorators may resolve these issues:

First, some background: I really want some settings to be "global" in the sense that they're not passed to the function, exactly. For example, I want to set up logging, globally, and I don't want to have logging configuration in every command. I use a custom namespace class/object with a cusutom setter to correspond to the logging option so that this happens automatically, but there's no way to add an argument everywhere that I can fined without a lot of duplicated decorators in a or that's makes semantic sense (and is functional).

So the issues are:

  1. I'd like to have settings that are global in scope that exist for their possible side-effect use. I'm thinking some sort of @arg variant like @arg_global that just adds the argument to to the parser but doesn't pass the value to the wrapped function during dispatch, would be awesome.
  2. I'd like to have some way to reduce redundancy in defining similar argument sets for different commands because duplication is annoying and error prone. I'm thinking about writing a decorator, or set of decorators that wraps entry_entry points with some basic options.

Thoughts?

@neithere
Copy link
Owner

neithere commented Jul 9, 2014

I haven't yet found time to dig into this but here's something that can be related: http://sourceforge.net/p/pgbarman/code/ci/master/tree/barman/cli.py

The Barman project uses Argh for CLI and:

  1. directly accesses argparse to add global arguments (such as --quiet and --debug);
  2. exploits the undocumented pre_call hook of the Argh dispatcher to load config, set up logging and such stuff (see global_config()).

I'm sure Argh should provide a documented way to do it but it may be better to gather use cases first. In my own CLI apps I stick to XDG + environ vars and that's usually enough. However, even these apps could be improved by a pre-dispatch setup function.

Warning

I must warn that the pre_call argument in argh.dispatch() does work but is undocumented and is not considered a public part of the API and therefore there will be no deprecation stage in case of removal. Of course in such case I will try to find any FOSS projects that depend on the feature and contact the authors but this is not very safe. So, if you do use this hook, please inform me of your use cases. Together we can design a better API.

neithere added a commit that referenced this issue Jul 9, 2014
@neithere
Copy link
Owner

Another interesting example: https://github.com/alfredhq/alfred/blob/develop/alfred/__main__.py (the @with_app decorator).

@ggaughan
Copy link

barman 1.3.3 seems to now break with argh 0.26 (pre_call seems to no longer work): https://sourceforge.net/p/pgbarman/tickets/56/

@neithere
Copy link
Owner

@ggaughan thanks, you are right. Fixed in 0.26.1.

@ggaughan
Copy link

Smashing! Thank you.

(https://sourceforge.net/p/pgbarman/tickets/56/)

@bukzor
Copy link

bukzor commented Jun 15, 2015

@neithere: that effectively promotes pre-call to API status. Is that intentional?

@jacobsvante
Copy link

jacobsvante commented Aug 25, 2016

Any news here? Some other use cases in addition to logging could be:

  • --dry-run
  • Hook in a pdb trace in case any of the sub-commands raise an exception, say with a --debug flag.

@neithere neithere added this to the 1.1 milestone Feb 8, 2023
neithere added a commit that referenced this issue Feb 14, 2023
This is not in fact deprecation per se but merely an attempt to draw
attention to a fact that was mentioned almost ten years ago in the
discussion (#63): the `pre_call` argument in `dispatch()` is not a
feature but a hack which is not recommended and will be removed.

This commit makes it clear when precisely it is going to be removed and
what actions are expected from the user.
@neithere
Copy link
Owner

FYI, the current hack (pre_call in dispatch()) is scheduled for removal in Argh v.0.30.

Its approval would promote an approach which would be against the purpose of the library, i.e. keeping things clear, natural and "pythonic". Argh is primarily a method of automatically building CLI from Python function signatures. The pre_call hack would effectively destroy the clear mapping.

There certainly should be a better, cleaner solution.

Some ideas:

  • A decorator for dispatching stage (see example above. The idea seems interesting but it's probably not currently functional after Fix #111: introspect args behind @wraps decorator #113. Still, some flag attached to the function should be enough to work around this.
  • A decorator for assembling stage. Requires more changes but is perhaps the cleanest option for reusing argument declarations, although it won't address shared calls unlike the one above.
  • An extended EntryPoint that would do the same.
  • Something involving partial parsing.

In any case, the fastest way to find a solution is to better define the problem. Gathering real use cases is perhaps the best way to move this issue forward. Perhaps there will be different solutions for seemingly identical problems.

neithere added a commit that referenced this issue Feb 15, 2023
* feat: deprecate outdated constant

* docs: list deprecated items

* test: cover argh.assembling.add_subcommands()

* test: cover overrides in add_commands()

* test: 100% coverage for assembling and interaction

* feat: deprecate `safe_input()`

The function used to be helpful in the py2/py3 world with all the bytes
vs Unicode strings mess.  In 2023 this is not relevant anymore.

* docs: deprecation of `dispatch(..., pre_call=...)`

This is not in fact deprecation per se but merely an attempt to draw
attention to a fact that was mentioned almost ten years ago in the
discussion (#63): the `pre_call` argument in `dispatch()` is not a
feature but a hack which is not recommended and will be removed.

This commit makes it clear when precisely it is going to be removed and
what actions are expected from the user.

* test: 100% coverage of dispatching

* chore: drop the old encoding declaration

Let's just agree that it's 2023 and move on.

* test: 100% coverage of completion

* test: require 100% coverage
@mathieulongtin
Copy link

First, thank for fixing a few niggles after a long hiatus. I appreciate it. We love argh.

We use pre_call in order to pre-load a configuration from the global flags before calling the function. This is built into a library that uses argh. Changing to the alfred style wouldn't work since it would force a change to all code that uses the library.

We can't use a custom action for the global flag, because it won't get called if the flag is not on the command line, and sometimes the configuration is in an environment variable.

Considering pre_call is an argument to dispatch and it is pretty clear what it does, I wouldn't consider it un-pythonic.

@neithere
Copy link
Owner

neithere commented Sep 21, 2023

Thank you for your kind words about Argh, @mathieulongtin.

Is that library open source? It would really help me to understand the use case if I could see the code.

I don't want to leave you stuck with an unsupported version of Argh, but I need to be sure that I understand the problem well and there's no other way to address the issue.

@neithere
Copy link
Owner

A bit more on this...

Known workflows

The normal workflow

  1. declare the function;
  2. infer CLI arguments from the function signature and its @arg-derived stack;
  3. create a subparser with these settings and attach the endpoint function to it;
  4. resolve CLI args into a namespace object (with endpoint function attached);
  5. extract function args from the namespace object to match function signature;
  6. call that function with its args extracted from the namespace object;
  7. process the return value.

The alfred workflow

Rather elegant but it drops one of the most powerful features of Argh — the mapping between CLI arguments and function signature.

  1. declare the function;
    🆕 Here we also wrap it into the @with_app decorator; it appends global args to the @arg-derived stack.
  2. infer CLI arguments from the function signature and its @arg-derived stack;
  3. create a subparser with these settings and attach the endpoint function to it;
  4. resolve CLI args into a namespace object (with endpoint function attached);
  5. 🆕 extract function args from the namespace object to match function signature;
  6. 🆕 call that function with its args extracted from the namespace object;
  7. process the return value.

The Barman workflow with pre_call

It's using "black magic", so it's definitely not the way to go:

  1. declare the function;
  2. 🆕 add global args to the parser;
  3. infer CLI arguments from the function signature and its @arg-derived stack;
  4. create a subparser with these settings and attach the endpoint function to it;
  5. resolve CLI args into a namespace object (with endpoint function attached);
    🆕 black magic: pre_call hook → monkey-patch another module's attribute based on the namespace object;
  6. extract function args from the namespace object to match function signature;
  7. call that function with its args extracted from the namespace object;
    🆕 black magic: the function accesses that monkey-patched module attribute to get its config.
  8. process the return value.

Some other pre_call workflow?

I'm not sure what approach @mathieulongtin is using and how exactly do the results of global args interpretation get available to the endpoint functions. Would be interesting to see an example.

Thoughts

Basically what we need is be able to inject some logic between resolving a namespace object + function and calling that function (ideally according to its signature). This whole part of the process is called dispatching (after we have assembled the parser with all of its endpoints).

There are however two problems which almost call for mutually exclusive solutions:

  • avoiding global arguments in endpoint function signatures;
  • passing the config object to endpoints.
    • no monkey-patching;
    • ideally no global objects;
    • no dedicated arg for the config object.
      • if the endpoint func expects the config to be passed as an argument, Argh needs to ignore that argument somehow when inferring CLI args from the signature
      • with the opposite (CLI→func args) — as of v.0.29 dispatching._execute_command() ignores unexpected func args but it also doesn't provide a mechanism to pass additional ones.

Possible solutions:

  • create a decorator, something like @config_arg, to tell Argh that a certain argument from the endpoint function signature should not be used to infer CLI args, but then obtained somehow and injected when the function is called.
    • this cannot work with the existing dispatch() method — it would need to know where to obtain that object. So it should be done manually or via a special hook, like pre_call but with a clean return value instead of sketchy fragile side effects.
  • use a class instance (say, App) which methods are used as commands; the instance itself will contain the shared state and Argh would ignore self by default.
    • note that it must be an instance, otherwise the function won't have access to its state when dispatched.
    • another problem — need to do one of the following:
      • post-configuration inside the dispatch phase:
        • add global args to the parser;
        • instantiate App;
        • assemble its bound methods into the parser (as subparsers);
        • parse all args → resolve into the endpoint function (bound App method);
        • configure the App instance;
        • call the function via _execute_command() — it will ignore the global args and have access to the configuration via self..
      • two-staged assemble/dispatch process:
        • parse_known_args() → a namespace with global args + remainder of CLI args;
        • instantiate (and configure) App;
        • assemble its bound methods into the parser;
        • parse the remainder of CLI args → resolve into the endpoint function;
        • call the function (like above).

@mathieulongtin
Copy link

mathieulongtin commented Sep 24, 2023

My use of pre_call is exactly the same as barman. The way I set it up, the sub-command are called after the configuration is loaded (the modifying something as a side effect). The pre-call is basically this:

def pre_call(ns):
   global CONFIG_NAME
   CONFIG_NAME = ns.config or os.environ.get("MYAPP_CONFIG")
   if not CONFIG_NAME: raise RuntimeError(...)
   load_config()

The way I see it, configuration is environmental, the same as the current working directory or environment variables. Having pre-call do what you call "monkey patch", I call it "setting the environment in an acceptable way for the functions to execute". This is especially true for a CLI,.

Moving along, there seems to be a need for pre_call. Here's how I would solve this: split dispatch() in two parts: parse_args and execute_command. Now pre_call is replaced by this:

ns = parse_args(parser)
# do what you'd do with pre-call
execute_function(parser, ns)

From the argh perspective, this is clean. Any kind of side-effect is clearly from the code using argh, not within argh.

neithere added a commit that referenced this issue Sep 28, 2023
Exceptions:

* `pre_call` was scheduled for removal in v0.30 but it needs to be
  replaced with some other mechanism, see discussion in #63.
@neithere neithere modified the milestones: 1.1, 0.30 Sep 28, 2023
neithere added a commit that referenced this issue Sep 28, 2023
* Remove previously deprecated code

Exception: `pre_call` was scheduled for removal in v0.30 but it needs to be
  replaced with some other mechanism, see discussion in #63.

* docs: update changelog

* chore: bump version
@neithere
Copy link
Owner

@mathieulongtin thanks for your input.
It makes sense to rearrange the internals in such a way that particular stages of the workflow would be usable directly, without hooks. I'll see if I can do it in the upcoming version or later.
The pre_call argument will not be removed until an alternative is available.

@neithere
Copy link
Owner

neithere commented Oct 2, 2023

@mathieulongtin does #193 look good for your use case? Please feel free to do the code review there or contribute the code. Targeting the upcoming release.

@mathieulongtin
Copy link

Looks good! It would be hard to refuse the exact API change I asked for :)

Thanks!

neithere added a commit that referenced this issue Oct 4, 2023
This addresses #184 while providing an alternative solution for #63.
neithere added a commit that referenced this issue Oct 21, 2023
* Remove previously deprecated code

Exception: `pre_call` was scheduled for removal in v0.30 but it needs to be
  replaced with some other mechanism, see discussion in #63.

* docs: update changelog

* chore: bump version
neithere added a commit that referenced this issue Oct 21, 2023
This addresses #184 while providing an alternative solution for #63.
@neithere
Copy link
Owner

Released in v0.30.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants