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

Trying to refactor CLI arguments parsing in order to support required, non-positional --app argument #441

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

tribals
Copy link

@tribals tribals commented Jun 8, 2024

Closes #439.

At this time, it contains only re-write of parsing to stdlib's argparse. You can test overall "look & feel" just by running waitress-serve with --help, and other arguments.

Not all parsing is done yet, but I think the whole Adjustments is not needed anymore - even argparse is powerful enough to handle common types, but "not so common" also - take a look at -4/-6 args (those was not there already, but they go in the style of ip from iproute2 - is it good thing at all?)

Another observation is that there is "boolean" options, but not all of them passed/works the same: some use --arg/--no-arg notation, but some works just as "if it passed then it's true" (but there also "reverse", notable example is --call). Should they all be reworked to --[no]-arg form, or something other?

@tribals tribals changed the title Trying to refactor CLI arguments parsing in order to support required, non-positional --app argument Draft: Trying to refactor CLI arguments parsing in order to support required, non-positional --app argument Jun 8, 2024
@tribals tribals changed the title Draft: Trying to refactor CLI arguments parsing in order to support required, non-positional --app argument Trying to refactor CLI arguments parsing in order to support required, non-positional --app argument Jun 8, 2024
@tribals tribals marked this pull request as draft June 8, 2024 22:58
@digitalresistor
Copy link
Member

I am not a fan of adding -4 and -6, you can already specify this by setting what address to bind to.

Also, Adjustments is also used when not using the runner directly, but using https://github.com/Pylons/waitress/blob/main/src/waitress/__init__.py#L6, so unless everything can go through the argparse changes you made, and all the code is updated that uses Adjustments currently (there's a lot of uses throughput the code base) I am not sure we can so easily get rid of it.

Remember that Waitress is not always run from the command line, but also run through things like Paste.

@tribals
Copy link
Author

tribals commented Jun 8, 2024

I am not a fan of adding -4 and -6...

The whole point is that they already exist in long form, so -4 is nothing more than a shortcut. Personally I argue that they not needed at all - as you said, one always specify that by passing address explicitly.

Remember that Waitress is not always run from the command line, but also run through things like Paste.

👌

@tribals
Copy link
Author

tribals commented Jun 22, 2024

I've finished porting args parsing to, you know, argparse.

I've not touched much Adjustments, and instead just passed already parsed arguments as is, without feeding them to "adjustments processing". On the other hand, this get's activated only when calling create_server from runner - other times everything works as it was.

It works! 😁

Also, I've fixed almost all tests, but some runner tests fail due to incompatibility of exit codes - I didn't find simple way to fix that.

@tribals tribals marked this pull request as ready for review June 26, 2024 20:59
Comment on lines +67 to +72
waitress-serve [OPTS] --app=MODULE:OBJECT

Required options:

``--app``
Specify WSGI application to run.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll break the existing external interface this way.

You should be able to accept the application using both the --app= flag and by passing it as a positional argument.

Copy link
Author

@tribals tribals Jul 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will - but first, I need a general agreement of how everything is implemented, and is argparse "enough" to solve the problem.

On the "positional app argument" - it is not clear now, should this argument be "still first", and for passing app eg. last one should use --app form. Or app should be accept eg. first or last - but this will complicate parsing.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On "argparse" side, right now there are some cases where it is hard to achieve desired results, eg. mutually exclusive groups of options. For now it can be solved as external "validation" step after parsing, and it will suffice, I think.

@digitalresistor
Copy link
Member

I am not opposed to this. I am going to need to chew on it for a little bit and then we may want to figure out how to make this work cleaner with the existing Adjustments and or make all of that cleaner. There's just a lot of tendrils in many different locations.

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

Successfully merging this pull request may close these issues.

Make MODULE:APP non-positional (but still required) argument
3 participants