Repository navigation
add additional exports to public API - #2910
Conversation
|
There are probably various other utility functions from https://github.com/sublimelsp/LSP-Tinymist/blob/5173ee56e777e3587f4823ef82107905e7445cd5/plugin.py#L22-L25 If we want to export more of them in the public API, perhaps it would be useful to reconsider some of the names, because there is a bit of inconsistency. A few function names are in the form "X_to_Y" ( Also the |
|
I think for backwards compatibility with packages that import @deprecated('Use TextPosition instead')
class Point(TextPosition):
pass |
Did that but not using new name in the code as I don't want to make things more difficult for @rwols. Can be renamed internally at later time. As for other utility functions and their inconsistent names, perhaps we could group those into some classes with just static functions. But then we'd have to come up with some logical grouping and naming which will probably take some time. |
Ok. But I think you still need to update a few references to the Point class inside of the TextPosition class itself. I would also rename the parameter in And as far as I can tell, the basic purpose of this class is to make the LSP Position comparable (by defining >>> from typing import TypedDict
>>> from typing import NamedTuple
>>> class Position(TypedDict):
... line: int
... character: int
...
>>> class TextPosition(NamedTuple):
... row: int
... col: int
... @classmethod
... def from_lsp(cls, position: Position) -> TextPosition:
... return TextPosition(position['line'], position['character'])
... def to_lsp(self) -> Position:
... return {'line': self.row, 'character': self.col}
...
>>> p1 = TextPosition.from_lsp({'line': 3, 'character': 5})
>>> p2 = TextPosition.from_lsp({'line': 3, 'character': 10})
>>> p1 < p2
True
>>> p1.to_lsp()
{'line': 3, 'character': 5}
>>> p1.row
3
>>> p3 = TextPosition.from_lsp({'line': 3, 'character': 5})
>>> p1 == p3
True |
|
That wouldn't have int coercion on init but I guess we don't really need that. Also it would pass Subclassing wouldn't work correctly for NamedTuple, though, I believe. |
|
It can be a dataclass with a caveat that comparing >>> from LSP.plugin.core.protocol import Point, TextPosition
>>> a = Point(1,1)
>>> b = TextPosition(1,1)
>>> a==b
False |
|
Perhaps you could merge with the latest changes from the main branch to run the handy new βtype-check community packagesβ GH action? |
|
All appears to pass. |
rwols
left a comment
rwols
left a comment
There was a problem hiding this comment.
I don't really have a big opinion on the function names. Looks good to me in any case.
|
|
||
|
|
||
| def point_to_offset(point: Point, view: sublime.View) -> int: | ||
| def point_to_offset(point: TextPosition, view: sublime.View) -> int: |
There was a problem hiding this comment.
Before we export these in the public API, could we also make the argument order consistent?
The exported offset_to_point, offset_to_position, region_to_range and text_document_position_params take the View as the first argument, while point_to_offset and position_to_offset have that as the second argument. I think it would be nice if all utility functions which take a View have it as the first argument.
Changing the argument order in point_to_offset would affect
- LSP-typescript
- LSP-julia
- LSP-rust-anylyzer
And in position_to_offset would affect
- LSP-copilot
- LSP-typescript
- LSP-tsgo
- LSP-some-sass
There was a problem hiding this comment.
If we want to maintain backwards compatibility for those helper packages for now, we could leave these two functions witch current argument order in views.py and mark them as deprecated, but also copy them with the updated argument order into another file. And then just import the updated functions from the new locations for the public API (in __init__.py). Later when all helper packages are updated to the new argument order, we could remove the deprecated functions and just move the updated ones back into views.py.
β Deploy Preview for sublime-lsp ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
β Deploy Preview for sublime-lsp ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
position_to_offsetPointLocationPickerLspExecuteCommandfirst_selection_regionpositionregion_to_rangetext_document_identifierpoint_to_offsettext_document_position_paramsI might keep this open for a while while I'm finding more things that should be public.