search

openlawlibrary / pygls

A pythonic generic language server
https://pygls.readthedocs.io/en/latest/
Apache License 2.0
563 stars 102 forks source link

handle ResponseErrors correctly #416

Closed dimbleby closed 9 months ago

dimbleby commented 9 months ago

The error on a ResponseErrorMessage is supposed to be a ResponseError, not a dictionary.

Older versions of cattrs seem to have tolerated the mistake, but newer ones don't.

The reason that this repository's own tests don't find this is that it continues to support python 3.7: and the versions of cattrs that don't like the mistake have dropped support for python 3.7. So poetry update here does not get the bug-exposing update.

However users who are on recent python versions do get the update, and do experience errors eg see https://github.com/pappasam/jedi-language-server/issues/296

A couple of meta-comments on this issue:

dimbleby commented 9 months ago

the test-pyodide CI check seems to be broken independent of this fix

tombh commented 9 months ago

Thank you.

The only reason we were holding back on dropping Python 3.7 was because VSCode promises 3 months of EOL support for each version. I think September 27th was the last day of that. So I'll submit a PR to drop 3.7.

Also, I think it might be worth disabling the Pyodide test for now. We've got some ideas to fix it. I wonder if it can just made to not fail the build.

dimbleby commented 9 months ago

Thanks.

If it is any trouble to this project to support python 3.7, then imo vscode's promises are not pygls's problem!

As the linked comment says: the implication of dropping python 3.7 support is anyway only that vscode would decline to take updates until those three months passed. Which presumably is what they are doing already with every other dependency that has dropped python 3.7 support.

Anyway I guess it's all academic now that everyone is happy to drop python 3.7.

It also occurred to me that this bug provided a (confusing) datapoint in the discussion about bounds on dependencies

tombh commented 9 months ago

Indeed, it didn't come up as a problem holding off on Python 3.7 deprecation, so I think it's all good. We'd obviously have to discuss it further if it had been.

Oh yeah, BTW, I totally agree about how this PR wouldn't have been needed if we'd better typing. My day job is in Golang and it feels a bit archaic coming back to this largely untyped codebase.

Thanks for mentioning that this is a datapoint in the discussion about bounds on dependencies. So even though cattrs doesn't follow semantic versioning would there still not be a pinnable version before which the bug was introduced. That way at least poetry.lock could be used as an at least "blessed" (rather than enforced) snapshot of working versions.

BTW we do actually have a Python version matrix setup on Github Actions, so in theory, if our poetry.lock did have the problematic version (as that is what is used to run the tests), then should we have seen the bug you found in the higher Python version matrix test runs?

dimbleby commented 9 months ago

poetry prefers to find a single version of all dependencies compatible with all supported python versions. So even though you have a 3.8 python in your CI you are still installing the cattrs version from poetry.lock, which is a version that is compatible with python 3.7.

If you dropped python 3.7 support before merging this fix (and remembered to poetry update) then you should have seen CI failures.

the "blessed" versions in poetry.lock do not help anyone outside of pygls developers: only the constraints from pyproject.toml make it to pypi, so everyone else who installs or uses pygls knows nothing about poetry.lock.

(This is an argument for not using poetry.lock in CI. The trade is: reproducible pipelines, vs seeing the bugs that your users will see)

tombh commented 9 months ago

poetry prefers to find a single version of all dependencies compatible with all supported python versions

Ahh yes of course, good point.

I like that framing of the lock/no-lock tradeoffs. Maybe we could have another CI step that always runs against latest versions?

dimbleby commented 9 months ago

Maybe we could have another CI step that always runs against latest versions?

That's not a thing I've seen done; but there's a logic to it. Depends whether you think it's worth the trouble I guess.

dimbleby commented 9 months ago

in fact cattrs is imported directly by pygls eg here so it should all along have been declared as a dependency in pyproject.toml

it still is entirely unclear what upper bound, if any, pygls ought to put on that dependency.

fxp0 commented 9 months ago

Is there any ETA for the next release with this fix?

tombh commented 9 months ago

We can release now. It's just nice to have fixes live on the main branch a while to test the waters.

PR for the release: #419

tombh commented 9 months ago

Released ✅