python-trio / flake8-async

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

New check: Use of deprecated feature (?) #134

Closed jakkdl closed 5 months ago

jakkdl commented 1 year ago

Split out from #121

New error: use of deprecated [function/method/etc], or can reuse TRIO117 now that there's a decent way of having multiple messages for the same error.

# deprecated anyio funcs
anyio_deprecated_funcs = (
    "anyio.abc.BlockingPortal.spawn_task",
    "anyio.abc.TaskGroup.spawn",
    "anyio.create_capacity_limiter",
    "anyio.create_condition",
    "anyio.create_event",
    "anyio.create_lock",
    "anyio.create_semaphore",
    "anyio.from_thread.create_blocking_bortal",
    "anyio.open_cancel_scope",
)

Are there any other trio deprecated funcs than multierror? non-explicit strict_exception_groups=True? (Or maybe that's a separate check for excepts without groups)

Zac-HD commented 1 year ago

Are there any other trio deprecated funcs than multierror?

This code suggests that the only other one is that trio.open_process() is deprecated in favor of trio.lowlevel.open_process() (the same function, in a new location).

I think we should deprecated passing strict_exception_groups=False, but otherwise just wait for upstream. In particular I don't want to require passing =True because you can pass it to trio.run() for global effect instead.

jakkdl commented 1 year ago

Should we lint on trio.run() without strict_exception_groups=True then?

jakkdl commented 1 year ago

Also, I'll let you press the assign button if you approve of adding it. Might wait with it until after libcst, even though it is going to be a very simple check.

Zac-HD commented 1 year ago

Should we lint on trio.run() without strict_exception_groups=True then?

In principle probably yes; in practice we monkeypatch it on so let's not 😜

jakkdl commented 1 year ago

Should we lint on trio.run() without strict_exception_groups=True then?

In principle probably yes; in practice we monkeypatch it on so let's not 😜

lmao. Still might be a useful check for other users of the plugin though - and I imagine a lot wouldn't even be aware of the flag until warned.

Zac-HD commented 1 year ago

Hmm, still inclined against. There are trade-offs which mean you might not want to yet, and it'll be the default behaviour at some point anyway.

jakkdl commented 1 year ago

Well, it's generally a good idea to stay ahead of future default behaviour changes, and in this case (once you're on 3.11) it seems like a clear upside. But makes sense to delay it until later, making it optional would probably not achieve much. One could make it non-optional with a good message and just expect users to disable the check if they want to stay on old behaviour for now, it's at least not going to be a noisy check since I'm guessing most programs only have a few calls to trio.run().

Zac-HD commented 1 year ago

The ecosystem needs a bit longer to catch up IMO; there are still quite a few libraries which don't actually handle ExceptionGroups coming out of a nursery... and of course that's a bug even in the non-strict case since it (almost) always could raise multiple errors. Unfortunately that's much more common under load than in unit tests!

jakkdl commented 5 months ago

Trio is good at raising TrioDeprecationWarning, so I don't think this merits the effort anymore.