nat-n / poethepoet

A task runner that works well with poetry.
https://poethepoet.natn.io/
MIT License
1.4k stars 58 forks source link

Use sys.argv[0] for help message USAGE line in ui.print_help(). #149

Closed sitbon closed 1 year ago

sitbon commented 1 year ago

Use case

I have a pyproject.toml within a packaged folder (with others in dynamic locations), and provide a global script which transparently runs poe inside a target folder to provide its tasks.

With this change, programs such as mine that wrap poe by using its main application class will automatically produce an accurate USAGE line.

Normal usage of poe will not see any change in the help message - regardless of whether it is running globally, via poetry, as a module, or as a poetry plugin.

Possible improvements to this change

PoeThePoet.__call__ could pass its internal flag down to ui.print_help() in order to control this behavior, or an optional program name field could be added to the class instead.

nat-n commented 1 year ago

Hi @sitbon, That's an interesting use case.

Your solution looks like it should work, but it's hard to be confident that it won't lead to weird results in some unforeseen circumstance.

For that reason I'm more inclined towards your other suggestion of passing an argument (like executable_name: str = "poe") via the PoeThePoet constructor to the PoeUi constructor. Do you seen any downside of taking that approach instead?

sitbon commented 1 year ago

One possible downside of passing an argument for the name is duplication of state, whereas sys.argv could be considered a source of truth for the running application -- such that any "weird result" could thus be considered as intentional if it's unexpected (not in ['poe', 'poetry', '__main__.py']).

This point wouldn't have come to mind had I not also discovered another place where the program name is needed, but is decoupled from both the main and ui classes.

With only two (or three?) places and two lines to get the name, it would be easy to just duplicate the code if you stick to using sys.argv. But now that I think about it, you have an opportunity here to do something a little more user-friendly: show the usage lines in the same form that they were invoked.

It would only require a function accessible from those two locations, which would be able to definitively reproduce the invocation (e.g. python -m <poethepoet-or-other-pkg> when sys.argv[0] == '__main__.py'). But if you were also juggling an executable_name field elsewhere, this feature might be less trivial to implement.

What do you think? If that sounds good, I can implement the function -- but I'm not entirely sure where it should live.

Btw, wrapping poe to provide tasks directly from my library & commands has been working flawlessly, and the one hangup I had was easily resolved with a custom executor :)

nat-n commented 1 year ago

It's nice to hear that using poe as a library worked well for you. I guess you can tell I factored the code with this in mind, but yours is the first real use case I know of.

You've partly convinced me. But still I'd prefer a slightly different approach consisting of the following:

  1. A function as you describe that infers the program name from argv, which could live in a new module like poethepoet.helpers.cli.py and should ideally have one or two unit tests.
  2. A new optional kwarg in the PoeThePoet class, that is passed to the PoeUI class and stored as a new public attribute on that class.
  3. Now in the UI class if the argument value is None (not provided) then it calls the helper function to infer a default value from argv.
  4. Two of the places the value needs to be used are in PoeUI where it can be referenced directly, and the other is in PoeTaskArgs which could also accept a new kwarg to the init method from PoeTask which can reference the value from the ui object.

Does that make sense?

What I like about this is that it maintains a single source of truth that is within the control of the application, and can be overridden by host code, but still tries to be smart by default.

One gotcha to be aware of in this is that when using the poetry plugin the poetry task name defaults to "poe" but can be overridden, or even empty! Properly handling this case would be a little more complex, probably specifying the value from the plugin code (e.g. "poetry poe") would make the most sense.

nat-n commented 1 year ago

Hey @sitbon , sorry for the friction on this.

I just got some time to pick this up and take it a bit further. I went with always using an explicitly configured value which is owned by the ui module. I prefer this solution because it doesn't add any logic to the default behaviour that could go wrong in some unforeseen circumstance, but still gives full control via the API.

While I was at it I also made it possible to configure PoeThePoet to load a different toml (or json) file instead of only pyproject.toml.

As a follow up I intend to add some documentation about using PoeThePoet as a library since now I see that's a real use case :)