openlawlibrary / pygls

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

cattrs is a direct dependency #421

Closed dimbleby closed 9 months ago

dimbleby commented 9 months ago

cattrs is imported as a direct dependency in several places, so this project should declare that dependency.

cattrs uses calendar versioning so there's no reason to expect that any choice of upper bound will be safe. I've guessed that you'll nevertheless prefer no upper bound over a pin, that seems to be the direction of travel in other issues and discussions.

alcarney commented 9 months ago

Honest question, why should we declare the dependency? Is it so that if lsprotocol dropped cattrs we'd be protected against that?

If I remember rightly (I could be wrong!) our direct usage of cattrs, is fairly minimal - mostly for type annotations where we integrate with lsprotocol.

I'm not intimately familiar with how dependency resolution works... would this declaration of cattrs override the transitive one in lsprotocol since it's... "closer" - meaning we'd have to stay on top of issues like microsoft/lsprotocol#301?

dimbleby commented 9 months ago

arguably this is a bit pedantic: pygls gets away without the direct dependency so long as there's an indirect dependency

but I think the implied burden of proof in your question is the wrong way round. pygls imports and uses cattrs, why would it not declare that as a requirement?

solvers are required to find a version of cattrs that is compatible both with whatever lsprotocol declares and with whatever pygls declares (and with whatever anything else in the tree declares)

This project is already on the hook to deal with changes or breakages in cattrs eg see #416. If anything this leaves you a little more ready to handle that by having control over the supported cattrs versions.

alcarney commented 9 months ago

pygls imports and uses cattrs, why would it not declare that as a requirement?

When you put it like that it makes perfect sense - thanks!

tombh commented 9 months ago

@alcarney do you have ability to merge into main?

alcarney commented 9 months ago

Yes, I think so, just wasn't sure which order you wanted to merge things since #417 touches the same files

tombh commented 9 months ago

No worries either way. I'm waiting for what's hopefully the final review from dimbleby on #417. So if you wanna merge now that's okay. If it generates a conflict it'll be easy to resolve.

tombh commented 9 months ago

Oh, I pipped you to the post 😬