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

refactor: rename server Position to PositionCodec, instantiate it in Workspace #383

Merged
merged 1 commit into from
Oct 2, 2023

Conversation

RossBencina
Copy link
Contributor

@RossBencina RossBencina commented Sep 29, 2023

Description

Implement the refactoring discussed in #382

  • Rename pygls Position type to PositionCodec
  • Create a single PositionCodec instance in Workspace that is used by all TextDocument instances
  • Add position_encoding and position_codec read-only properties to Workspace
  • Make position_codec a read-only property of TextDocument
  • Improve deprecation warning messages associated with the old position mapping functions that have been replaced by PositionCodec

Resolves #382

Code review checklist (for code reviewer to complete)

  • Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR)
  • Title summarizes what is changing
  • Commit messages are meaningful (see this for details)
  • Tests have been included and/or updated, as appropriate
  • Docstrings have been included and/or updated, as appropriate
  • Standalone docs have been updated accordingly

@RossBencina RossBencina force-pushed the position_codec_refactor branch from 5f8e598 to 8929a7d Compare September 29, 2023 09:32
@RossBencina
Copy link
Contributor Author

Fixed lint checks

@RossBencina
Copy link
Contributor Author

There is a question as to whether we also want to roll back the recent changes that qualified lsprotocol symbols with types. to avoid collisions between the two Position types e.g. in e271ba5

If that's desirable I propose to do it in a separate PR.

@RossBencina RossBencina force-pushed the position_codec_refactor branch from 8929a7d to 06ebe97 Compare September 29, 2023 09:51
@RossBencina
Copy link
Contributor Author

fixed position = PositionCodec to codec = PositionCodec in test_document.py

Apologies for the forced updates. I am done now.

@RossBencina
Copy link
Contributor Author

RossBencina commented Sep 29, 2023

Failing CI/test-pyodide but so is the main branch. The reported problem looks unrelated to pygls.

@tombh
Copy link
Collaborator

tombh commented Sep 29, 2023

This looks great! Thank you very much.

I think in general throughout the codebase we're trying to move towards qualifying all lsprotocol types with types.* anyway.

I'm a big fan of force pushing in PRs, so thanks for that too.

Yeah don't worry about the Pyodide test.

You can select me or @alcarney, or both, to review this.

@RossBencina RossBencina force-pushed the position_codec_refactor branch from 06ebe97 to 96a6eaa Compare September 29, 2023 19:41
@RossBencina
Copy link
Contributor Author

^^^ force push: make all deprecation warnings consistent:

diff --git a/pygls/workspace/__init__.py b/pygls/workspace/__init__.py
index a18f9d5..4880ef4 100644
--- a/pygls/workspace/__init__.py
+++ b/pygls/workspace/__init__.py
@@ -17,9 +17,9 @@ Document = TextDocument

 def utf16_unit_offset(chars: str):
     warnings.warn(
-        "'utf16_unit_offset' has been deprecated, use "
+        "'utf16_unit_offset' has been deprecated, instead use "
         "'PositionCodec.utf16_unit_offset' via 'workspace.position_codec' "
-        "or 'text_document.position_codec' instead",
+        "or 'text_document.position_codec'",
         DeprecationWarning,
         stacklevel=2,
     )

@RossBencina
Copy link
Contributor Author

You can select me or @alcarney, or both, to review this.

Thanks for the feedback! I don't believe that I have permissions to assign reviewers. I'm happy for both or either of you to review.

@RossBencina RossBencina force-pushed the position_codec_refactor branch from 96a6eaa to e65c193 Compare September 30, 2023 05:49
@RossBencina
Copy link
Contributor Author

^^^ rebased to latest main

Copy link
Collaborator

@alcarney alcarney left a comment

Choose a reason for hiding this comment

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

Looks good to me :)

@RossBencina RossBencina mentioned this pull request Oct 1, 2023
6 tasks
@tombh tombh merged commit e62421a into openlawlibrary:main Oct 2, 2023
17 checks passed
@tombh
Copy link
Collaborator

tombh commented Oct 2, 2023

Thank you! ❤️

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.

Poor experience with recent "Position" changes
3 participants