-
Notifications
You must be signed in to change notification settings - Fork 46
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
Use document path instead of an instance #265
Conversation
Current dependencies on/for this PR: This comment was auto-generated by Graphite. |
if document is None or document.path is None: | ||
return list(WORKSPACE_SETTINGS.values())[0] | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part becomes irrelevant as it's never executed.
ruff_lsp/server.py
Outdated
@@ -1013,8 +1013,7 @@ def _update_workspace_settings(settings: list[WorkspaceSettings]) -> None: | |||
} | |||
|
|||
|
|||
def _get_document_key(document: workspace.TextDocument) -> str | None: | |||
document_workspace = Path(document.path) | |||
def _get_document_key(document_workspace: Path) -> str | None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we not pass a Document
in here and then call Path
on it internally? What's the motivation for passing in Path
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, for notebook we have a different type NotebookDocument
. The motivation being that the event handler will pass in either the notebook or file path along with the source code to the _lint_document_impl
. The source code for a notebook will be a JSON string which we need to construct as per the schema (only the required fields).
We could create a type NotebookOrTextDocument
which would abstract out the path
and source
field since that's the only thing we use. Or, we could use a Protocol
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm actually thinking of using str
here instead and use Path
whenever required instead of instantiating it early on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm actually thinking of using str
here instead and use Path
whenever required instead of instantiating it early on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, what you have here seems reasonable.
8e3603f
to
e114eea
Compare
e114eea
to
1b58531
Compare
Summary
The
TextDocument
instance will always exists, so we can remove the| None
from the type signature. The reason being that
pygls
will always return aTextDocument
instance viaworkspace.get_text_document
method.Also, the
document.path
field can never beNone
so the check is useless aswell.