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

upgrade pygls #373

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

upgrade pygls #373

wants to merge 7 commits into from

Conversation

ekacnet
Copy link

@ekacnet ekacnet commented Jan 14, 2024

  • Update tests to deal with the upgrade of pygls
  • Adapt server code to new version of pylsp
  • Ensure version is never None
  • Bump the version of pygls
  • Remove locked version
  • Expose version of the package and use it for the LspServer

@ekacnet
Copy link
Author

ekacnet commented Jan 14, 2024

Would be great if the PR was not purely squashed but instead merged on top of current main to preserve the different commit ...

@ekacnet ekacnet force-pushed the upgrade_pygls branch 2 times, most recently from ed18188 to 5bbcbca Compare January 15, 2024 03:48
@dcermak
Copy link
Owner

dcermak commented Jan 31, 2024

Thank you for this massive PR and apologies for taking so long to respond.

This lgtm overall, however, I'd like to not remove the poetry.lock file as that ensures that everyone gets the same environment and not some random package version.

Additionally, your code now no longer formats cleanly. Please run poetry run black to fix that. And I saw that you added isort for import sorting. I am not opposed to that, but please enforce it then via the CI.

If you don't want to address all these changes yourself, then let me know and I'll cherry-pick & add the requested changes myself.

Mainly it's fixing the import so that the new names are used also
replace {put/get}_document by {put/get}_text_document
0.11 is kind of outdated so let's get something more recent
@codecov-commenter
Copy link

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (a91e55e) 76.73% compared to head (df30424) 76.51%.
Report is 3 commits behind head on main.

Files Patch % Lines
salt_lsp/__init__.py 66.66% 2 Missing ⚠️
salt_lsp/workspace.py 92.30% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #373      +/-   ##
==========================================
- Coverage   76.73%   76.51%   -0.23%     
==========================================
  Files           8        9       +1     
  Lines         735      745      +10     
  Branches      147      138       -9     
==========================================
+ Hits          564      570       +6     
- Misses        141      145       +4     
  Partials       30       30              
Flag Coverage Δ
python-3.10 76.24% <90.90%> (-0.23%) ⬇️
python-3.8 76.24% <90.90%> (-0.23%) ⬇️
python-3.9 76.24% <90.90%> (-0.23%) ⬇️
unittests 76.37% <90.90%> (-0.23%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -206,9 +213,9 @@ def setup_custom_workspace(self):
"""
if not isinstance(self.workspace, SlsFileWorkspace):
old_ws = self.workspace
self.workspace = SlsFileWorkspace(
self._workspace = SlsFileWorkspace(
Copy link
Owner

Choose a reason for hiding this comment

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

This change will break all the functions that assume that SaltLspProto.workspace is an instance of SlsFileWorkspace

@@ -168,7 +169,8 @@ def initialize(params: InitializeParams) -> None:
server.logger.debug("Replaced workspace with SlsFileWorkspace")
Copy link
Owner

Choose a reason for hiding this comment

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

mypy is now complaining about line 168:

salt_lsp/server.py:168: error: "LanguageServerProtocol" has no attribute "setup_custom_workspace"  [attr-defined]

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