Repository: openlawlibrary/pygls (python)

Issue:
Poor experience with recent "Position" changes
I have just been bitten hard by recent changes and I'd like to offer a critique and suggest some improvements:

The problem that I hit is that up until now I have been using `utf16_num_units` to convert between python string indexes and client position units (I am doing operational-transform like processing on document changes, so need this low level capability). With recent updates, I get the following error:

```
DeprecationWarning: 'utf16_num_units' has been deprecated, use 'Position.client_num_units' instead
```

I think I understand the intent of the message, but here are some of the problems that I encountered:

- `Position` up until now has meant `lsprotocol.types.Position`. After a thorough search, I find `lsprotocol.types.Position` has no `client_num_units` method, nor has such a method been monkey patched into the class. So the deprecation message is confusing.
- Then I started looking at recent commits and discover that a second (!) `Position` class has been added: `pygls.workspace.position.Position`. Introducing a type name collision on such a fundamental domain entity seems crazy to me.
- To make matters even more confusing, `pygls.workspace.position.Position` does not actually model *positions* it models *encoding-specific position translation*, so a better name might be `PositionTranslator` or  `PositionCodec`
- Now, the deprecation warning says "use `Position.client_num_units` instead" but `pygls.workspace.position.Position.client_num_units` is not a class method -- you can't use that function directly, you need to find an instance of it (where?). I guessed at `workspace.position` but it is not there. Later I found `text_document.position`

So, my suggestions are:

- Rename `pygls.workspace.position.Position` to `pygls.workspace.position_codec.PositionCodec` or maybe `PositionTranslator`
- store an instance of `PositionCodec` in the `Workspace` and `TextDocument` instances, so that `workspace.position_codec` and `text_document.position_codec` can be easily used. I would understand if you decided against adding the `workspace` field but really, there needs to be only a single codec for the entire workspace, it doesn't need to be instantiated on a per-text-document basis.
- Fix the DeprecationWarnings in a way that makes them accurate and helpful, for example:

```
DeprecationWarning: 'utf16_num_units' has been deprecated, use 'text_document.position_codec.client_num_units' instead
```









Pull request:
refactor: rename server Position to PositionCodec, instantiate it in Workspace
## 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][commit messages] 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

[commit messages]: https://conventionalcommits.org/


Patch:
diff --git a/pygls/workspace/__init__.py b/pygls/workspace/__init__.py
index afa2590..4880ef4 100644
--- a/pygls/workspace/__init__.py
+++ b/pygls/workspace/__init__.py
@@ -5,11 +5,11 @@ from lsprotocol import types
 
 from .workspace import Workspace
 from .text_document import TextDocument
-from .position import Position
+from .position_codec import PositionCodec
 
 Workspace = Workspace
 TextDocument = TextDocument
-Position = Position
+PositionCodec = PositionCodec
 
 # For backwards compatibility
 Document = TextDocument
@@ -17,65 +17,71 @@ Document = TextDocument
 
 def utf16_unit_offset(chars: str):
     warnings.warn(
-        "'utf16_unit_offset' has been deprecated, use "
-        "'Position.utf16_unit_offset' instead",
+        "'utf16_unit_offset' has been deprecated, instead use "
+        "'PositionCodec.utf16_unit_offset' via 'workspace.position_codec' "
+        "or 'text_document.position_codec'",
         DeprecationWarning,
         stacklevel=2,
     )
-    _position = Position()
-    return _position.utf16_unit_offset(chars)
+    _codec = PositionCodec()
+    return _codec.utf16_unit_offset(chars)
 
 
 def utf16_num_units(chars: str):
     warnings.warn(
-        "'utf16_num_units' has been deprecated, use "
-        "'Position.client_num_units' instead",
+        "'utf16_num_units' has been deprecated, instead use "
+        "'PositionCodec.client_num_units' via 'workspace.position_codec' "
+        "or 'text_document.position_codec'",
         DeprecationWarning,
         stacklevel=2,
     )
-    _position = Position()
-    return _position.client_num_units(chars)
+    _codec = PositionCodec()
+    return _codec.client_num_units(chars)
 
 
 def position_from_utf16(lines: List[str], position: types.Position):
     warnings.warn(
-        "'position_from_utf16' has been deprecated, use "
-        "'Position.position_from_client_units' instead",
+        "'position_from_utf16' has been deprecated, instead use "
+        "'PositionCodec.position_from_client_units' via "
+        "'workspace.position_codec' or 'text_document.position_codec'",
         DeprecationWarning,
         stacklevel=2,
     )
-    _position = Position()
-    return _position.position_from_client_units(lines, position)
+    _codec = PositionCodec()
+    return _codec.position_from_client_units(lines, position)
 
 
 def position_to_utf16(lines: List[str], position: types.Position):
     warnings.warn(
-        "'position_to_utf16' has been deprecated, use "
-        "'Position.position_to_client_units' instead",
+        "'position_to_utf16' has been deprecated, instead use "
+        "'PositionCodec.position_to_client_units' via "
+        "'workspace.position_codec' or 'text_document.position_codec'",
         DeprecationWarning,
         stacklevel=2,
     )
-    _position = Position()
-    return _position.position_to_client_units(lines, position)
+    _codec = PositionCodec()
+    return _codec.position_to_client_units(lines, position)
 
 
 def range_from_utf16(lines: List[str], range: types.Range):
     warnings.warn(
-        "'range_from_utf16' has been deprecated, use "
-        "'Position.range_from_client_units' instead",
+        "'range_from_utf16' has been deprecated, instead use "
+        "'PositionCodec.range_from_client_units' via "
+        "'workspace.position_codec' or 'text_document.position_codec'",
         DeprecationWarning,
         stacklevel=2,
     )
-    _position = Position()
-    return _position.range_from_client_units(lines, range)
+    _codec = PositionCodec()
+    return _codec.range_from_client_units(lines, range)
 
 
 def range_to_utf16(lines: List[str], range: types.Range):
     warnings.warn(
-        "'range_to_utf16' has been deprecated, use "
-        "'Position.range_to_client_units' instead",
+        "'range_to_utf16' has been deprecated, instead use "
+        "'PositionCodec.range_to_client_units' via 'workspace.position_codec' "
+        "or 'text_document.position_codec'",
         DeprecationWarning,
         stacklevel=2,
     )
-    _position = Position()
-    return _position.range_to_client_units(lines, range)
+    _codec = PositionCodec()
+    return _codec.range_to_client_units(lines, range)
diff --git a/pygls/workspace/position.py b/pygls/workspace/position_codec.py
similarity index 98%
rename from pygls/workspace/position.py
rename to pygls/workspace/position_codec.py
index 0f4616d..8189cfb 100644
--- a/pygls/workspace/position.py
+++ b/pygls/workspace/position_codec.py
@@ -25,7 +25,7 @@ from lsprotocol import types
 log = logging.getLogger(__name__)
 
 
-class Position:
+class PositionCodec:
     def __init__(
         self,
         encoding: Optional[
@@ -121,7 +121,9 @@ class Position:
                 break
 
             _current_char = _line[utf32_index]
-            _is_double_width = Position.is_char_beyond_multilingual_plane(_current_char)
+            _is_double_width = PositionCodec.is_char_beyond_multilingual_plane(
+                _current_char
+            )
             if _is_double_width:
                 if self.encoding == types.PositionEncodingKind.Utf32:
                     _client_index += 1
diff --git a/pygls/workspace/text_document.py b/pygls/workspace/text_document.py
index 27b300a..d62c6aa 100644
--- a/pygls/workspace/text_document.py
+++ b/pygls/workspace/text_document.py
@@ -20,12 +20,12 @@ import io
 import logging
 import os
 import re
-from typing import List, Optional, Pattern, Union
+from typing import List, Optional, Pattern
 
 from lsprotocol import types
 
 from pygls.uris import to_fs_path
-from .position import Position
+from .position_codec import PositionCodec
 
 # TODO: this is not the best e.g. we capture numbers
 RE_END_WORD = re.compile("^[A-Za-z_0-9]*")
@@ -43,9 +43,7 @@ class TextDocument(object):
         language_id: Optional[str] = None,
         local: bool = True,
         sync_kind: types.TextDocumentSyncKind = types.TextDocumentSyncKind.Incremental,
-        position_encoding: Optional[
-            Union[types.PositionEncodingKind, str]
-        ] = types.PositionEncodingKind.Utf16,
+        position_codec: Optional[PositionCodec] = None,
     ):
         self.uri = uri
         self.version = version
@@ -65,11 +63,15 @@ class TextDocument(object):
         )
         self._is_sync_kind_none = sync_kind == types.TextDocumentSyncKind.None_
 
-        self.position = Position(encoding=position_encoding)
+        self._position_codec = position_codec if position_codec else PositionCodec()
 
     def __str__(self):
         return str(self.uri)
 
+    @property
+    def position_codec(self) -> PositionCodec:
+        return self._position_codec
+
     def _apply_incremental_change(
         self, change: types.TextDocumentContentChangeEvent_Type1
     ) -> None:
@@ -78,7 +80,7 @@ class TextDocument(object):
         text = change.text
         change_range = change.range
 
-        range = self.position.range_from_client_units(lines, change_range)
+        range = self._position_codec.range_from_client_units(lines, change_range)
         start_line = range.start.line
         start_col = range.start.character
         end_line = range.end.line
@@ -165,11 +167,13 @@ class TextDocument(object):
     def offset_at_position(self, client_position: types.Position) -> int:
         """Return the character offset pointed at by the given client_position."""
         lines = self.lines
-        server_position = self.position.position_from_client_units(
+        server_position = self._position_codec.position_from_client_units(
             lines, client_position
         )
         row, col = server_position.line, server_position.character
-        return col + sum(self.position.client_num_units(line) for line in lines[:row])
+        return col + sum(
+            self._position_codec.client_num_units(line) for line in lines[:row]
+        )
 
     @property
     def source(self) -> str:
@@ -217,7 +221,7 @@ class TextDocument(object):
         if client_position.line >= len(lines):
             return ""
 
-        server_position = self.position.position_from_client_units(
+        server_position = self._position_codec.position_from_client_units(
             lines, client_position
         )
         row, col = server_position.line, server_position.character
diff --git a/pygls/workspace/workspace.py b/pygls/workspace/workspace.py
index 1ae2528..746c1ba 100644
--- a/pygls/workspace/workspace.py
+++ b/pygls/workspace/workspace.py
@@ -30,6 +30,7 @@ from lsprotocol.types import (
 )
 from pygls.uris import to_fs_path, uri_scheme
 from pygls.workspace.text_document import TextDocument
+from pygls.workspace.position_codec import PositionCodec
 
 logger = logging.getLogger(__name__)
 
@@ -60,11 +61,20 @@ class Workspace(object):
         self._folders: Dict[str, WorkspaceFolder] = {}
         self._docs: Dict[str, TextDocument] = {}
         self._position_encoding = position_encoding
+        self._position_codec = PositionCodec(encoding=position_encoding)
 
         if workspace_folders is not None:
             for folder in workspace_folders:
                 self.add_folder(folder)
 
+    @property
+    def position_encoding(self) -> Optional[Union[PositionEncodingKind, str]]:
+        return self._position_encoding
+
+    @property
+    def position_codec(self) -> PositionCodec:
+        return self._position_codec
+
     def _create_text_document(
         self,
         doc_uri: str,
@@ -78,7 +88,7 @@ class Workspace(object):
             version=version,
             language_id=language_id,
             sync_kind=self._sync_kind,
-            position_encoding=self._position_encoding,
+            position_codec=self._position_codec,
         )
 
     def add_folder(self, folder: WorkspaceFolder):


Test patch:
diff --git a/tests/test_document.py b/tests/test_document.py
index 859f508..f071a2f 100644
--- a/tests/test_document.py
+++ b/tests/test_document.py
@@ -19,7 +19,7 @@
 import re
 
 from lsprotocol import types
-from pygls.workspace import TextDocument, Position
+from pygls.workspace import TextDocument, PositionCodec
 from .conftest import DOC, DOC_URI
 
 
@@ -174,71 +174,71 @@ def test_document_source_unicode():
 
 
 def test_position_from_utf16():
-    position = Position(encoding=types.PositionEncodingKind.Utf16)
-    assert position.position_from_client_units(
+    codec = PositionCodec(encoding=types.PositionEncodingKind.Utf16)
+    assert codec.position_from_client_units(
         ['x="😋"'], types.Position(line=0, character=3)
     ) == types.Position(line=0, character=3)
-    assert position.position_from_client_units(
+    assert codec.position_from_client_units(
         ['x="😋"'], types.Position(line=0, character=5)
     ) == types.Position(line=0, character=4)
 
 
 def test_position_from_utf32():
-    position = Position(encoding=types.PositionEncodingKind.Utf32)
-    assert position.position_from_client_units(
+    codec = PositionCodec(encoding=types.PositionEncodingKind.Utf32)
+    assert codec.position_from_client_units(
         ['x="😋"'], types.Position(line=0, character=3)
     ) == types.Position(line=0, character=3)
-    assert position.position_from_client_units(
+    assert codec.position_from_client_units(
         ['x="😋"'], types.Position(line=0, character=4)
     ) == types.Position(line=0, character=4)
 
 
 def test_position_from_utf8():
-    position = Position(encoding=types.PositionEncodingKind.Utf8)
-    assert position.position_from_client_units(
+    codec = PositionCodec(encoding=types.PositionEncodingKind.Utf8)
+    assert codec.position_from_client_units(
         ['x="😋"'], types.Position(line=0, character=3)
     ) == types.Position(line=0, character=3)
-    assert position.position_from_client_units(
+    assert codec.position_from_client_units(
         ['x="😋"'], types.Position(line=0, character=7)
     ) == types.Position(line=0, character=4)
 
 
 def test_position_to_utf16():
-    position = Position(encoding=types.PositionEncodingKind.Utf16)
-    assert position.position_to_client_units(
+    codec = PositionCodec(encoding=types.PositionEncodingKind.Utf16)
+    assert codec.position_to_client_units(
         ['x="😋"'], types.Position(line=0, character=3)
     ) == types.Position(line=0, character=3)
 
-    assert position.position_to_client_units(
+    assert codec.position_to_client_units(
         ['x="😋"'], types.Position(line=0, character=4)
     ) == types.Position(line=0, character=5)
 
 
 def test_position_to_utf32():
-    position = Position(encoding=types.PositionEncodingKind.Utf32)
-    assert position.position_to_client_units(
+    codec = PositionCodec(encoding=types.PositionEncodingKind.Utf32)
+    assert codec.position_to_client_units(
         ['x="😋"'], types.Position(line=0, character=3)
     ) == types.Position(line=0, character=3)
 
-    assert position.position_to_client_units(
+    assert codec.position_to_client_units(
         ['x="😋"'], types.Position(line=0, character=4)
     ) == types.Position(line=0, character=4)
 
 
 def test_position_to_utf8():
-    position = Position(encoding=types.PositionEncodingKind.Utf8)
-    assert position.position_to_client_units(
+    codec = PositionCodec(encoding=types.PositionEncodingKind.Utf8)
+    assert codec.position_to_client_units(
         ['x="😋"'], types.Position(line=0, character=3)
     ) == types.Position(line=0, character=3)
 
-    assert position.position_to_client_units(
+    assert codec.position_to_client_units(
         ['x="😋"'], types.Position(line=0, character=4)
     ) == types.Position(line=0, character=6)
 
 
 def test_range_from_utf16():
-    position = Position(encoding=types.PositionEncodingKind.Utf16)
-    assert position.range_from_client_units(
+    codec = PositionCodec(encoding=types.PositionEncodingKind.Utf16)
+    assert codec.range_from_client_units(
         ['x="😋"'],
         types.Range(
             start=types.Position(line=0, character=3),
@@ -253,7 +253,7 @@ def test_range_from_utf16():
         start=types.Position(line=0, character=3),
         end=types.Position(line=0, character=5),
     )
-    actual = position.range_from_client_units(['x="😋😋"'], range)
+    actual = codec.range_from_client_units(['x="😋😋"'], range)
     expected = types.Range(
         start=types.Position(line=0, character=3),
         end=types.Position(line=0, character=4),
@@ -262,8 +262,8 @@ def test_range_from_utf16():
 
 
 def test_range_to_utf16():
-    position = Position(encoding=types.PositionEncodingKind.Utf16)
-    assert position.range_to_client_units(
+    codec = PositionCodec(encoding=types.PositionEncodingKind.Utf16)
+    assert codec.range_to_client_units(
         ['x="😋"'],
         types.Range(
             start=types.Position(line=0, character=3),
@@ -278,7 +278,7 @@ def test_range_to_utf16():
         start=types.Position(line=0, character=3),
         end=types.Position(line=0, character=4),
     )
-    actual = position.range_to_client_units(['x="😋😋"'], range)
+    actual = codec.range_to_client_units(['x="😋😋"'], range)
     expected = types.Range(
         start=types.Position(line=0, character=3),
         end=types.Position(line=0, character=5),
@@ -300,56 +300,64 @@ def test_offset_at_position_utf16():
 
 
 def test_offset_at_position_utf32():
-    doc = TextDocument(DOC_URI, DOC, position_encoding=types.PositionEncodingKind.Utf32)
+    doc = TextDocument(
+        DOC_URI,
+        DOC,
+        position_codec=PositionCodec(encoding=types.PositionEncodingKind.Utf32),
+    )
     assert doc.offset_at_position(types.Position(line=0, character=8)) == 8
     assert doc.offset_at_position(types.Position(line=5, character=0)) == 39
 
 
 def test_offset_at_position_utf8():
-    doc = TextDocument(DOC_URI, DOC, position_encoding=types.PositionEncodingKind.Utf8)
+    doc = TextDocument(
+        DOC_URI,
+        DOC,
+        position_codec=PositionCodec(encoding=types.PositionEncodingKind.Utf8),
+    )
     assert doc.offset_at_position(types.Position(line=0, character=8)) == 8
     assert doc.offset_at_position(types.Position(line=5, character=0)) == 41
 
 
 def test_utf16_to_utf32_position_cast():
-    position = Position(encoding=types.PositionEncodingKind.Utf16)
+    codec = PositionCodec(encoding=types.PositionEncodingKind.Utf16)
     lines = ["", "😋😋", ""]
-    assert position.position_from_client_units(
+    assert codec.position_from_client_units(
         lines, types.Position(line=0, character=0)
     ) == types.Position(line=0, character=0)
-    assert position.position_from_client_units(
+    assert codec.position_from_client_units(
         lines, types.Position(line=0, character=1)
     ) == types.Position(line=0, character=0)
-    assert position.position_from_client_units(
+    assert codec.position_from_client_units(
         lines, types.Position(line=1, character=0)
     ) == types.Position(line=1, character=0)
-    assert position.position_from_client_units(
+    assert codec.position_from_client_units(
         lines, types.Position(line=1, character=2)
     ) == types.Position(line=1, character=1)
-    assert position.position_from_client_units(
+    assert codec.position_from_client_units(
         lines, types.Position(line=1, character=3)
     ) == types.Position(line=1, character=2)
-    assert position.position_from_client_units(
+    assert codec.position_from_client_units(
         lines, types.Position(line=1, character=4)
     ) == types.Position(line=1, character=2)
-    assert position.position_from_client_units(
+    assert codec.position_from_client_units(
         lines, types.Position(line=1, character=100)
     ) == types.Position(line=1, character=2)
-    assert position.position_from_client_units(
+    assert codec.position_from_client_units(
         lines, types.Position(line=3, character=0)
     ) == types.Position(line=2, character=0)
-    assert position.position_from_client_units(
+    assert codec.position_from_client_units(
         lines, types.Position(line=4, character=10)
     ) == types.Position(line=2, character=0)
 
 
 def test_position_for_line_endings():
-    position = Position(encoding=types.PositionEncodingKind.Utf16)
+    codec = PositionCodec(encoding=types.PositionEncodingKind.Utf16)
     lines = ["x\r\n", "y\n"]
-    assert position.position_from_client_units(
+    assert codec.position_from_client_units(
         lines, types.Position(line=0, character=10)
     ) == types.Position(line=0, character=1)
-    assert position.position_from_client_units(
+    assert codec.position_from_client_units(
         lines, types.Position(line=1, character=10)
     ) == types.Position(line=1, character=1)