handle ResponseErrors correctly

[dimbleby]

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 pappasam/jedi-language-server#296

A couple of meta-comments on this issue:

• probably it's time to drop support for python 3.7 here: much of the world has already done so, and by continuing to support it pygls is not seeing updates that its users are seeing
• it's another example where adding type annotations would have caught the bug before it ever happened

[dimbleby]

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

[tombh]

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]

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

• clearly it is an example of a dependency releasing a breaking change, causing non-obvious bugs for users
• on the other hand cattrs is not a direct dependency, and does not use semantic versioning: so it is unlikely that pygls would ever have pinned it in a way that avoided this

[tombh]

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]

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]

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]

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]

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]

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

[tombh]

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]

Released ✅