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

Unify type annotations in uwsgiconf.uwsgi_stub with types-uWSGI #8

Open
Daverball opened this issue Jul 24, 2023 · 2 comments
Open

Comments

@Daverball
Copy link

Hi there,
types-uWSGI has been accepted and merged into typeshed today (it should be available on pypi by tomorrow)

First of all, let me thank you for your work. It's been great having someone else's stubs to double check my own stubs against. It's quicker than double checking the C-Code, where it's easy to miss an argument here or there or misread whether or not it's a default argument. It helped me find a handful of errors I've made when creating the stubs, but I've also discovered quite a few errors in your stubs along the way.

So I'd recommend checking your stubs against types-uWSGI. One of the big things that you got wrong, is that you annotated all the arguments as named, when they're actually positional only arguments, if you don't care about supporting 3.7 or lower, then you can put a / at the end of each argument list, or if you do, then you can use the workaround I used in the stubs of prefixing the argument names with a double underscore.

The other big thing is, that you weren't using the 2.0.21 release tag as a reference, but master, which contains some new functions/arguments that are not part of 2.0.21, I've made the same mistake initially, but figured it out when I was trying to add annotations for the request_context argument on the websocket functions.

One relatively quick way to compare the stubs, rather than to do it manually, would probably be to copy uwsgi.pyi from types-uWSGI into uwsgi_stub.pyi and use stubtest which lets you test a pyi file against the runtime types in a module.

@idlesign
Copy link
Owner

Hi.
Thank you.

One of the big things that you got wrong, is that you annotated all the arguments as named [...]

That's not strictly so. Current annotation can be described as positional-or-keyword. Cf. keyword-only using *,.
Positional-only (using /) is a maybe feature. Thank you for the idea.

The other big thing is, that you weren't using the 2.0.21 release tag as a reference, but master, [...]

That one is deliberate, since not everybody uses the released versions.

@Daverball
Copy link
Author

Daverball commented Jul 25, 2023

That's not strictly so. Current annotation can be described as positional-or-keyword. Cf. keyword-only using *,. Positional-only (using /) is a maybe feature. Thank you for the idea.

Yes, they will work if everyone uses them as positional arguments, the problem is just if someone uses them as keyword arguments they will get neither a runtime nor type checking error if they're using uwsgi_stub, but will get a runtime error if they use the real uwsgi module. It shouldn't come up that often, but I still think it's still a good idea to err on the side of correctness here, so your type checker actually helps you catch this error early.

That one is deliberate, since not everybody uses the released versions.

Fair enough. It seems a little bit dangerous to be this loose about which version of the package goes with which version of uWSGI, but I suppose most of the changes are negligible currently, so maintaining two separate versions might not be worth it.

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

2 participants