python-trio / flake8-async

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

Add TRIO114: unknown startable #69

Closed jakkdl closed 1 year ago

jakkdl commented 1 year ago

fixes #66 Branched from #68 - so the diff is a bit ugly. The manual test added should probably have a helper function.

I'm not entirely happy with the logic here, it now checks function names only against the last part of any specified pattern - which means that a startable function foo in module bar, that's only ever used in the code as bar.foo, will never prompt you to add it if there's a pattern for other_module.foo. Shared function names across modules/classes is pretty common, so means you can't relax and rely on TRIO114 to remind you every time you forget a method.

Possible solutions:

  1. Do a more sophisticated check, if the function name is foo, and the specified pattern is zee.foo check if we're in module zee, or we're a class method, otherwise error. If the pattern is just foo or *.foo it's satisfied.
  2. Keep track of which option names, not including wildcards, have been matched previously. So if we encounter two function definitions with the name foo, and there's only one pattern specifying 'modulename.foo', be unhappy?

Not super happy with either of those. Could maybe make it dumber and give a flag entering a verbose mode where it will print every time a positive match is made (instead of warning when there's no match) - meant to be inspected manually by the end-user every now and then. So e.g. prints

function foo in file path/to/bar.py matches pattern random_other_module.foo That means trio114 can stay dumb and warn on obvious missing patterns, but you can every now and then run with this flag and double check that all your methods are properly matched.

(TRIO113 could similarly also do that, to catch cases where modules are imported as aliases, or class methods are passed).

Zac-HD commented 1 year ago

I'm not entirely happy with the logic here, it now checks function names only against the last part of any specified pattern - which means that a startable function foo in module bar, that's only ever used in the code as bar.foo, will never prompt you to add it if there's a pattern for other_module.foo. Shared function names across modules/classes is pretty common, so means you can't relax and rely on TRIO114 to remind you every time you forget a method.

I'm actually OK with this; in practice all my patterns consist of either a bare name or *.name, which is handled. More generally, the error model for TRIO114 is "contributor didn't know or forgot to add the new startable thing", which I think is still pretty well covered.

As a backup manual check, grepping for task_status.*=trio\.TASK_STATUS_IGNORED and cross-referencing the config works well - I did that for my code, and then thought of this new check when considering what would happen to the list as other people add new code 🙂

Zac-HD commented 1 year ago

✅ Found a function that my grepping had missed, though registering it didn't trigger any new TRIO113 warnings 😁

jakkdl commented 1 year ago

I'm not entirely happy with the logic here, it now checks function names only against the last part of any specified pattern - which means that a startable function foo in module bar, that's only ever used in the code as bar.foo, will never prompt you to add it if there's a pattern for other_module.foo. Shared function names across modules/classes is pretty common, so means you can't relax and rely on TRIO114 to remind you every time you forget a method.

I'm actually OK with this; in practice all my patterns consist of either a bare name or *.name, which is handled. More generally, the error model for TRIO114 is "contributor didn't know or forgot to add the new startable thing", which I think is still pretty well covered.

As a backup manual check, grepping for task_status.*=trio\.TASK_STATUS_IGNORED and cross-referencing the config works well - I did that for my code, and then thought of this new check when considering what would happen to the list as other people add new code slightly_smiling_face

right, makes sense. I guess you're not importing startable functions across modules (without using from import ) too much then. It might make sense to add this to the readme then as a best practice, and/or suggest using grep.