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

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

Merged
merged 2 commits into from
Jul 8, 2023

Conversation

sitbon
Copy link
Contributor

@sitbon sitbon commented May 19, 2023

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.

@sitbon sitbon force-pushed the feat-program-name branch from c49439b to d91f021 Compare May 19, 2023 22:26
@nat-n
Copy link
Owner

nat-n commented May 20, 2023

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
Copy link
Contributor Author

sitbon commented May 23, 2023

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
Copy link
Owner

nat-n commented May 24, 2023

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 nat-n added the enhancement New feature or request label Jul 4, 2023
@nat-n nat-n force-pushed the development branch 2 times, most recently from 57c3a0e to ae5ed47 Compare July 4, 2023 20:32
This improves support for customizing the behavior of PoeThePoet when using it
as a library.

Also:
- fix sphinx linkcheck error with a GitHub link
- add poe task to (re)install the poetry plugin to help with testing
@nat-n nat-n force-pushed the feat-program-name branch from 2a7351d to 80f7e1b Compare July 8, 2023 20:34
@nat-n
Copy link
Owner

nat-n commented Jul 8, 2023

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 :)

@nat-n nat-n merged commit 0260f1d into nat-n:development Jul 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants