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

pygls update #86

Merged
merged 3 commits into from
Mar 16, 2021
Merged

pygls update #86

merged 3 commits into from
Mar 16, 2021

Conversation

dimbleby
Copy link
Contributor

Not for merging, certainly until pygls next releases.

I've been wondering since #78 (comment) whether openlawlibrary/pygls#139 was going to be a good thing, and at the weekend I kind of got sucked into going down the path of finding out.

This - and prompting that project to merge their longstanding PR - is the result.

Obviously not for merging until there's an actual pygls release and pyproject.toml can be updated to point at that, rather than to a local checkout. But I figured I should get this in front of you sooner rather than later in case you hate it, or want changes, or were about to duplicate the work, or whatever.

I made a series of commits:

  • first just updating to new pygls, mostly mechanical, allowed removal of a couple of hokey workarounds
  • then noticed that pygls gave convenient access to ClientCapabilities, so removed them from the initialise-params-parser
  • then decided that with pydantic now in the dependency tree anyway, initialization options could be very conveniently handled by using that, I think it's turned out nicely. So nicely that there was really nothing left to unit test, not sure whether that'll be controversial!

What else interesting? Oh, bumped min python version to 3.6.1, some of our dependencies need that to be able to update to recent versions and I think it should be uncontroversial: surely very few even of those who are stuck on python 3.6 are stuck on python 3.6.0.

@pappasam
Copy link
Owner

Wow, this looks great! At a glance, I have no major issues with the direction you've taken this. I'll wait for pygls to release the next version and then immediately start testing out this PR / giving you any functionality-based feedback that I find. Thanks for pushing this ahead, I'm very pleased with all the simplifications you've produced!

@pappasam
Copy link
Owner

@dimbleby looks like pygls was just released, let me know when you believe this is mergeable and I'll so some deeper tests then.

@dimbleby
Copy link
Contributor Author

I re-pushed to use pygls 0.10.0, should be good for a closer look now.

@dimbleby
Copy link
Contributor Author

actually I've made a tiny mess of rewriting the history and my inner perfectionist isn't quite happy, will fix shortly.

@dimbleby dimbleby changed the title WIP: pygls update pygls update Mar 16, 2021
@dimbleby
Copy link
Contributor Author

I've just noticed pygls_type_overrides.py.

That looks as though it might be workaround for things that no longer need workaround, though I'm not sure exactly what. Can we remove that too, do you think?

@dimbleby
Copy link
Contributor Author

Yes, looks like this went in for #38 and was a particular case of the thing where pygls liked to send null. But it doesn't do that now, so will remove this too.

@dimbleby
Copy link
Contributor Author

Oh, and also the private reimplementation of RenameFile, surely can remove that too.

@pappasam
Copy link
Owner

This PR relates to openlawlibrary/pygls#151

Copy link
Owner

@pappasam pappasam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@pappasam pappasam merged commit 5d6d3ce into pappasam:master Mar 16, 2021
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.

2 participants