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.

Sign up for GitHub

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: utilize latest lsp_utils #33

Merged
merged 11 commits into from
Nov 28, 2020
Merged

refactor: utilize latest lsp_utils #33

Changes from 10 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
19a6a4a
refactor: use on_settings_changed() for ST 4
jfcherng Nov 24, 2020
512e6f7
fix: don't append extraPaths twice
jfcherng Nov 24, 2020
13eab63
fix: DottedDict's values are nested dicts
jfcherng Nov 24, 2020
62df060
refactor: adopt changes from sublimelsp/lsp_utils#39
jfcherng Nov 25, 2020
75ae679
chore: add annoatation
jfcherng Nov 25, 2020
82edca3
refactor: use API decorator (on_notification)
jfcherng Nov 25, 2020
5136bcb
chore: remove dead codes
jfcherng Nov 25, 2020
30ccbed
fixup: wrong API decorator name
jfcherng Nov 25, 2020
44f174d
chore: nits
jfcherng Nov 25, 2020
798bc6b
chore: use a more general way to get package_name
jfcherng Nov 25, 2020
ef7c4e4
fix: wrong import path for notification_handler
jfcherng Nov 25, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 26 additions & 18 deletions plugin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
import os
import sublime
import sys

from LSP.plugin import DottedDict
from LSP.plugin import notification_handler
jfcherng marked this conversation as resolved.
Show resolved Hide resolved
from LSP.plugin.core.typing import Any, Dict, List, Optional, Tuple
from lsp_utils import NpmClientHandler
from sublime_lib import ActivityIndicator
import os
import sublime
import sys


def plugin_loaded() -> None:
Expand All @@ -16,7 +17,7 @@ def plugin_unloaded() -> None:


class LspPyrightPlugin(NpmClientHandler):
package_name = __package__
package_name = __package__.split(".")[0]
rchl marked this conversation as resolved.
Show resolved Hide resolved
server_directory = "language-server"
server_binary_path = os.path.join(server_directory, "node_modules", "pyright", "langserver.index.js")

Expand All @@ -33,38 +34,45 @@ def install_in_cache(cls) -> bool:
def minimum_node_version(cls) -> Tuple[int, int, int]:
return (12, 0, 0)

@classmethod
def on_settings_read(cls, settings: sublime.Settings) -> bool:
super().on_settings_read(settings)
def on_settings_changed(self, settings: DottedDict) -> None:
super().on_settings_changed(settings)

if settings.get("dev_environment") == "sublime_text":
server_settings = settings.get("settings", {}) # type: Dict[str, Any]
if self.get_plugin_setting("dev_environment") == "sublime_text":
Copy link
Member

@rchl rchl Nov 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This might sound like a crazy idea but this get_plugin_setting wouldn't be necessary had we put our dev_environment inside the settings object.

The server will ignore unknown properties anyway and we'll get the benefit of merged settings automatically (not that important in this case but could be in other cases).

Notice that it's already happening (in VSCode at least) as the python settings are not really pyright specific in VSCode and the server still receives all of them.

Copy link
Collaborator Author

@jfcherng jfcherng Nov 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Not against that but probably adding an extra prefix like sublimelsp.dev_environment?

Copy link
Member

@rchl rchl Nov 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I don't feel it's really necessary. We should treat those settings as ours as that's how normal users see them. Users don't need to know the distinction between our and original pyright settings.
IMO.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

having a dedicated prefix makes it more structured imho. now we only have one settings though.

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with a prefix but not sure what it would be.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I propose using the package name as the prefix, i.e., LSP-pyright.dev_environment.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry but it sounds very redundant to put a LSP-pyright.* key inside the LSP-pyright.sublime-settings or inside the LSP-pyright object in project settings, for example.

I think it only achieves peace of mind for you, and possibly other developers working on the plugin, because you want to keep the settings of pyright and our additions separate. But as I said, I don't believe those should be treated as separate. To normal users, there is no distinction really.

That setting is a bit special as it affects/modifies other settings though. So from that perspective, I guess it might make sense to single it out somehow. But I really can't think of anything better right now. Personally, I would think that a name like environment_type would be enough.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That setting is a bit special as it affects/modifies other settings though. So from that perspective, I guess it might make sense to single it out somehow.

Does it imply only dev_environment (whatever its name) will be in server settings but not other non-server settings in the future?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think that implies anything as we don't have strict rules for this. I'm just trying to think of what's best to do here.

But in the end, it's a trivial thing and we should probably not spend too much time discussing it.

Maybe just keep as it is now, even if it complicates the code internally...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah. I guess that can be discussed later if wanted. https://github.com/neoclide/coc-python#options seems to just use python as the prefix.

server_settings = settings.get() # type: Dict[str, Any]

# add package dependencies into "python.analysis.extraPaths"
extraPaths = server_settings.get("python.analysis.extraPaths", []) # type: List[str]
extraPaths.extend(cls.find_package_dependency_dirs())
extraPaths.extend(self.find_package_dependency_dirs())
server_settings["python.analysis.extraPaths"] = extraPaths

settings.set("settings", server_settings)

return False
settings.update(server_settings)

def on_ready(self, api) -> None:
api.on_notification("pyright/beginProgress", self.handle_begin_progress)
api.on_notification("pyright/endProgress", self.handle_end_progress)
api.on_notification("pyright/reportProgress", self.handle_report_progress)
# ---------------- #
# message handlers #
# ---------------- #

@notification_handler("pyright/beginProgress")
def handle_begin_progress(self, params) -> None:
# we don't know why we begin this progress
# the reason will be updated in "pyright/reportProgress"
self._start_indicator("{}: Working...".format(self.package_name))

@notification_handler("pyright/endProgress")
def handle_end_progress(self, params) -> None:
self._stop_indicator()

@notification_handler("pyright/reportProgress")
def handle_report_progress(self, params: List[str]) -> None:
self._start_indicator("{}: {}".format(self.package_name, "; ".join(params)))

# -------------- #
# custom methods #
# -------------- #

@classmethod
def get_plugin_setting(cls, key: str, default: Optional[Any] = None) -> Any:
return sublime.load_settings(cls.package_name + ".sublime-settings").get(key, default)

@staticmethod
def find_package_dependency_dirs() -> List[str]:
dep_dirs = sys.path.copy()
Expand Down