Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cattrs is a direct dependency #421

Closed
wants to merge 1 commit into from
Closed

cattrs is a direct dependency #421

wants to merge 1 commit into from

Conversation

dimbleby
Copy link
Contributor

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
Copy link
Collaborator

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?

alcarney
alcarney previously approved these changes Nov 30, 2023
@dimbleby
Copy link
Contributor Author

dimbleby commented Nov 30, 2023

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
Copy link
Collaborator

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
Copy link
Collaborator

tombh commented Nov 30, 2023

@alcarney do you have ability to merge into main?

@alcarney
Copy link
Collaborator

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

@tombh
Copy link
Collaborator

tombh commented Nov 30, 2023

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
Copy link
Collaborator

tombh commented Nov 30, 2023

Oh, I pipped you to the post 😬

@dimbleby dimbleby closed this by deleting the head repository Nov 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants