python-trio / flake8-async

Highly opinionated linter for Trio code
https://flake8-async.readthedocs.io
MIT License
17 stars 2 forks source link

Simplify `startable-in-context-manager` setting by using unqualified name instead of fnmatch pattern #72

Closed Zac-HD closed 1 year ago

Zac-HD commented 1 year ago

Turns out that this is what I actually want in practice, and it would be simpler and more robust to configuration errors.

I'm also pretty comfortable recommending that if you have multiple functions or methods with the same name, where some are startable and others aren't, you should probably rename some of them! [aside: yes, I've recently renamed some functions 😅]

Per https://github.com/Zac-HD/flake8-trio/pull/69#issuecomment-1330486212 it'd also be good to have the readme suggest using git grep -E 'task_status.*=trio.TASK_STATUS_IGNORED' to fill out the list initially, and suggest that inspecting call sites for await trio.sleep() can be useful in identifying functions which aren't startable but perhaps could be.

jakkdl commented 1 year ago

I'm guessing only for parameters added by the users, and not trio's built-ins?

But could add serve as an unqualified name https://trio.readthedocs.io/en/stable/reference-io.html#trio.DTLSEndpoint.serve

Zac-HD commented 1 year ago

Hmm. Maybe we should just include the trio-provided functions too on the same principle? "Thou shalt not have name collisions between startable and non-startable functions" seems OK, actually!

And yeah, adding serve sounds good to me - "start serving" is a reasonable expectation and e.g. hypercorn.trio.serve is definitely startable.

jakkdl commented 1 year ago

Should the option manager raise an error if there's a . in the parameter list? Explicit dotted names could still be supported (with/without wildcards) - and since we require that you import trio normally it makes some sense to specifically check trio.run_process and not just run_process. In that case the difference would merely be in how an undotted name is handled and otherwise current functionality is kept.

(I also thought it was going to be a pain to implement this check decently, but the issue comment box worked well as a rubber duck for my tired head ^^)

Zac-HD commented 1 year ago

Let's error on anything for which str.isidentifier returns False, since that's the exact requirement to be a valid unqualified name.

My point above is that if we just check for run_process, this will catch your own custom implementations too and I think this is good; if you don't want yours to be assumed startable IMO you should pick a different name that doesn't collide with one of Trio's startable functions. (and you can always # noqa if it's a third-party issue)