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

Add LSP-caos #115

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add LSP-caos #115

wants to merge 1 commit into from

Conversation

bedalton
Copy link

Copy link

@STReviewBot STReviewBot left a comment

Choose a reason for hiding this comment

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

Automated testing result: WARNING

Repo link: LSP-caos
Results help

Packages added:
  - LSP-caos

Processing package "LSP-caos"
  - WARNING: The package does not contain a top-level LICENSE file. A license helps users to contribute to the package.

@predragnikolic
Copy link
Member

predragnikolic commented Jun 24, 2024

Hello @bedalton

I took a look. Here are some things I found:

1. This section from the README.md should be removed/changed.

Format on save

To format your code on save add the following setting to your syntax-specific settings (Elixir in this case) and/or project files:

{
  "lsp_format_on_save": true
}

2. LSP-caos doesn't install.

I am not sure why this happens. I do not have yarn locally.

[lsp_utils] START output of command: "import"

[lsp_utils] Command output END
Unable to start subprocess for LSP-caos
Traceback (most recent call last):
  File "/Users/predrag/Library/Application Support/Sublime Text/Lib/python38/lsp_utils/server_npm_resource.py", line 123, in install_or_update
    self._node_runtime.run_install(cwd=self._server_dest)
  File "/Users/predrag/Library/Application Support/Sublime Text/Lib/python38/lsp_utils/node_runtime.py", line 405, in run_install
    self._run_yarn(['import'], cwd)
  File "/Users/predrag/Library/Application Support/Sublime Text/Lib/python38/lsp_utils/node_runtime.py", line 445, in _run_yarn
    raise Exception('Failed to run yarn command "{}":\n{}'.format(' '.join(args), error))
Exception: Failed to run yarn command "import":
yarn import v1.22.21
info found npm package-lock.json, converting to yarn.lock
error Failed to import from package-lock.json, source file(s) corrupted
info Visit https://yarnpkg.com/en/docs/cli/import for documentation about this command.

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/Users/predrag/Library/Application Support/Sublime Text/Packages/LSP/plugin/core/windows.py", line 259, in start_async
    plugin_class.install_or_update()
  File "/Users/predrag/Library/Application Support/Sublime Text/Lib/python38/lsp_utils/_client_handler/abstract_plugin.py", line 109, in install_or_update
    server.install_or_update()
  File "/Users/predrag/Library/Application Support/Sublime Text/Lib/python38/lsp_utils/server_npm_resource.py", line 127, in install_or_update
    raise Exception('Error installing the server:\n{}'.format(error))
Exception: Error installing the server:
Failed to run yarn command "import":
yarn import v1.22.21

3. It would be nice if ST had an better API to style regions of code...

I kind of dislike this section of the code, but I understand that you had to do it to apply styles differently in the view:

def update_plugin_settings():
    """
    Watch for plugin settings changes
    """
    global last_color_scheme
    remove_generated_color_schemes()
    last_color_scheme = None
    create_color_scheme()

4. Add a licence


Once you fix some things, don't forget to just tag a new release.

@rchl
Copy link
Member

rchl commented Jun 25, 2024

Instead of global plugin_path variable:

https://github.com/bedalton/LSP-caos/blob/d7d078de5b5ad577cfc9e3ccab1559ae061eada4/plugin.py#L17-L18

you should do something like this:

https://github.com/sublimelsp/LSP-pyright/blob/7a91dc9917006010b1811f0602d7ffd0831629df/plugin.py#L38-L40

Or alternatively just assign a variable without using global. You don't need it because you are not modifying that variable from any code later. And it is a good convention to use UPPER_CASE for such constants.

@rchl
Copy link
Member

rchl commented Jun 25, 2024

Shipping syntax files with LSP packages is generally not recommended because people might want to use syntaxes without LSP itself. Have you considered publishing those separately?

As far as I see there already exists 6 year old syntax for something like that. Would it make sense to replace it with yours if yours is more modern / complete?

@rchl
Copy link
Member

rchl commented Jun 25, 2024

Having this option here doesn't do anything because it's an LSP option that should go in personal LSP settings:

https://github.com/bedalton/LSP-caos/blob/d7d078de5b5ad577cfc9e3ccab1559ae061eada4/LSP-caos.sublime-settings#L8

@bedalton
Copy link
Author

bedalton commented Jul 7, 2024

Shipping syntax files with LSP packages is generally not recommended because people might want to use syntaxes without LSP itself. Have you considered publishing those separately?

As far as I see there already exists 6 year old syntax for something like that. Would it make sense to replace it with yours if yours is more modern / complete?

Is there a way to make sure the syntax file is installed along with my LSP package? As far as I can tell, other plugins make highlighting possible without needed to install another package with the syntax file.

@rchl
Copy link
Member

rchl commented Aug 24, 2024

Many packages just state in the readme the requirement of having to install a particular syntax. There is no way to set up automatic dependency between two packages.

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.

4 participants