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 ruff linter plugin #1

Merged
merged 9 commits into from
Dec 10, 2022
Merged

Add ruff linter plugin #1

merged 9 commits into from
Dec 10, 2022

Conversation

jhossbach
Copy link
Member

WIP for the implementation of ruff linting for pylsp (see this discussion)
It passes the unit tests (as adapted from the flake8 unit tests).

I did come across one very peculiar error from ruff:

Error running ruff: error: Found argument '--line-length' which wasn't expected, or isn't valid in this context

  If you tried to supply '--line-length' as a value rather than a flag, use '-- --line-length'

Usage: ruff <FILES|--config <CONFIG>|--verbose|--quiet|--silent|--exit-zero|--watch|--fix|--no-cache|--select <SELECT>|--extend-select <EXTEND_SELECT>|--ignore <IGNORE>|--extend-ignore <EXTEND_IGNORE>|--exclude <EXCLUDE>|--extend-exclude <EXTEND_EXCLUDE>|--per-file-ignores <PER_FILE_IGNORES>|--format <FORMAT>|--show-files|--show-settings|--add-noqa|--dummy-variable-rgx <DUMMY_VARIABLE_RGX>|--target-version <TARGET_VERSION>|--autoformat|--stdin-filename <STDIN_FILENAME>> <--verbose|--quiet|--silent>

For more information try '--help'

Do you have any idea @charliermarsh? I have no MWE so far and could not reproduce this using the CLI (which should work exactly the same compared to subprocess.Popen). Maybe an old version?

I can upload the plugin to PyPi as soon as the PR is approved and merged.

@jhossbach jhossbach requested a review from ccordoba12 December 5, 2022 16:54
Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

Thanks a lot @jhossbach for your work on this! I left some simple style suggestions to improve docstrings, otherwise looks good to me.

In case you haven't done it before, please check this guide to apply all my suggestions in a single commit by adding them to the batch.

pylsp_ruff/ruff_lint.py Outdated Show resolved Hide resolved
pylsp_ruff/ruff_lint.py Outdated Show resolved Hide resolved
pylsp_ruff/ruff_lint.py Outdated Show resolved Hide resolved
pylsp_ruff/ruff_lint.py Outdated Show resolved Hide resolved
pylsp_ruff/ruff_lint.py Outdated Show resolved Hide resolved
pylsp_ruff/ruff_lint.py Outdated Show resolved Hide resolved
pylsp_ruff/ruff_lint.py Outdated Show resolved Hide resolved
pylsp_ruff/ruff_lint.py Show resolved Hide resolved
pylsp_ruff/ruff_lint.py Outdated Show resolved Hide resolved
.github/workflows/python.yml Outdated Show resolved Hide resolved
@ccordoba12 ccordoba12 added this to the v1.0.0 milestone Dec 8, 2022
@charliermarsh
Copy link

How do the LSP plugins find the Ruff executable? Do they expect it to be globally available? Or do users point the LSP to an interpreter?

Co-authored-by: Carlos Cordoba <[email protected]>
@jhossbach
Copy link
Member Author

How do the LSP plugins find the Ruff executable? Do they expect it to be globally available? Or do users point the LSP to an interpreter?

You can provide a path to your ruff executable through pylsp.plugins.ruff.executable, if nothing is provided the plugin will assume it to be available globally. We can change this by importing ruff directly as a python module once astral-sh/ruff#659 is implemented.
I'm open to suggestions on how to improve this if necessary :)

Co-authored-by: Carlos Cordoba <[email protected]>
@jhossbach jhossbach merged commit f767ea9 into main Dec 10, 2022
@charliermarsh
Copy link

@jhossbach - Who's responsible for publishing to PyPI? Let me know when it's live and I can give it a try in neovim!

@jhossbach
Copy link
Member Author

@jhossbach - Who's responsible for publishing to PyPI? Let me know when it's live and I can give it a try in neovim!

On it 👍

@jhossbach
Copy link
Member Author

@charliermarsh: The PyPi package is published: https://pypi.org/project/python-lsp-ruff/1.0.0/
Let me know if you have any troubles, thanks a lot for testing!

@jhossbach jhossbach deleted the ruff_linter branch December 10, 2022 19:12
@charliermarsh
Copy link

@jhossbach - For some reason I'm getting this:

[ERROR][2022-12-10 15:00:59] .../vim/lsp/rpc.lua:733	"rpc"	"pylsp"	"stderr"	"2022-12-10 15:00:59,445 EST - ERROR - pylsp_ruff.ruff_lint - Error running ruff: error: Found argument '--isort' which wasn't expected, or isn't valid in this context\n\n  If you tried to supply '--isort' as a value rather than a flag, use '-- --isort'\n\nUsage: ruff <FILES|--config <CONFIG>|--verbose|--quiet|--silent|--exit-zero|--watch|--fix|--no-fix|--no-cache|--select <SELECT>|--extend-select <EXTEND_SELECT>|--ignore <IGNORE>|--extend-ignore <EXTEND_IGNORE>|--exclude <EXCLUDE>|--extend-exclude <EXTEND_EXCLUDE>|--fixable <FIXABLE>|--unfixable <UNFIXABLE>|--per-file-ignores <PER_FILE_IGNORES>|--format <FORMAT>|--show-source|--show-files|--show-settings|--add-noqa|--dummy-variable-rgx <DUMMY_VARIABLE_RGX>|--target-version <TARGET_VERSION>|--line-length <LINE_LENGTH>|--max-complexity <MAX_COMPLEXITY>|--autoformat|--stdin-filename <STDIN_FILENAME>|--explain <EXPLAIN>|--generate-shell-completion <SHELL>> <--verbose|--quiet|--silent>\n\nFor more information try '--help'\n\n"

...in the Neovim LSP logs. Any ideas?

@jhossbach
Copy link
Member Author

Any ideas?

Hmm, can you try increasing the log level of pylsp when starting the lsp via nvim? If you use nvim-lspconfig you can specify the command with

require("lspconfig").pylsp.setup{ cmd={<path_to_pylsp>,  '-vvv', '--log-file', '/tmp/pylsp.log' } }

@charliermarsh
Copy link

Ahh, ok:

2022-12-10 15:17:56,629 EST - DEBUG - pylsp_ruff.ruff_lint - Got ruff settings: {'ignore': None, 'perFileIgnores': None, 'executable': 'ruff', 'lineLength': None, 'enabled': True, 'select': None, 'exclude': None, 'config': None}
2022-12-10 15:17:56,630 EST - DEBUG - pylsp_ruff.ruff_lint - Found pyproject file: /Users/crmarsh/workspace/ruff/./pyproject.toml
2022-12-10 15:17:56,633 EST - DEBUG - pylsp_ruff.ruff_lint - XXX
2022-12-10 15:17:56,633 EST - DEBUG - pylsp_ruff.ruff_lint - {'config': None, 'exclude': None, 'ignore': None, 'line-length': None, 'select': None, 'isort': {'force-wrap-aliases': True, 'combine-as-imports': True}}
2022-12-10 15:17:56,633 EST - DEBUG - pylsp_ruff.ruff_lint - ['--quiet', '--format=json', "--isort={'force-wrap-aliases': True, 'combine-as-imports': True}", '--', '-']
2022-12-10 15:17:56,633 EST - DEBUG - pylsp_ruff.ruff_lint - Calling ruff with args: ['--quiet', '--format=json', "--isort={'force-wrap-aliases': True, 'combine-as-imports': True}", '--', '-'] on '/Users/crmarsh/workspace/ruff/setup.py'
2022-12-10 15:17:56,908 EST - ERROR - pylsp_ruff.ruff_lint - Error running ruff: error: Found argument '--isort' which wasn't expected, or isn't valid in this context

  If you tried to supply '--isort' as a value rather than a flag, use '-- --isort'

Usage: ruff <FILES|--config <CONFIG>|--verbose|--quiet|--silent|--exit-zero|--watch|--fix|--no-fix|--no-cache|--select <SELECT>|--extend-select <EXTEND_SELECT>|--ignore <IGNORE>|--extend-ignore <EXTEND_IGNORE>|--exclude <EXCLUDE>|--extend-exclude <EXTEND_EXCLUDE>|--fixable <FIXABLE>|--unfixable <UNFIXABLE>|--per-file-ignores <PER_FILE_IGNORES>|--format <FORMAT>|--show-source|--show-files|--show-settings|--add-noqa|--dummy-variable-rgx <DUMMY_VARIABLE_RGX>|--target-version <TARGET_VERSION>|--line-length <LINE_LENGTH>|--max-complexity <MAX_COMPLEXITY>|--autoformat|--stdin-filename <STDIN_FILENAME>|--explain <EXPLAIN>|--generate-shell-completion <SHELL>> <--verbose|--quiet|--silent>

For more information try '--help'

@charliermarsh
Copy link

Most of the settings options in pyproject.toml aren't supported as command-line arguments (and in this case, it's not properly handling the nested options, like those under isort).

Instead of trying to pass in pyproject.toml options as command-line arguments like this:

# Load config from pyproject file if it exists
if pyproject_file:
  try:
      log.debug(f"Found pyproject file: {str(pyproject_file[0])}")

      with open(str(pyproject_file[0]), "rb") as pyproject_toml:
          toml_dict = tomllib.load(pyproject_toml)

      toml_config = toml_dict.get("tool", {}).get("ruff", {})

      # Update settings with local project settings
      for key, value in toml_config.items():
          settings[key] = value

  except (tomllib.TOMLDecodeError, OSError) as e:
      log.warning(f"Failed loading {str(pyproject_file)}: {e}")

...could we instead just pass --config=/path/to/pyproject.toml to Ruff, and let Ruff do the parsing of the pyproject.toml?

@charliermarsh
Copy link

(That may not even be necessary because Ruff will automatically find the nearest pyproject.toml in a parent directory? Unless Neovim intentionally has different behavior around workspaces and stuff.)

@jhossbach
Copy link
Member Author

Good point, I also thought about letting ruff find the pyproject.toml file without the help of pylsp. I can't think of any reason why this would be necessary, but the same is done for python-lsp-black (which also finds the toml file as far as I know?)
Do you have any idea if/why pylsp needs to read the per-project config @ccordoba12?

@jhossbach
Copy link
Member Author

@charliermarsh what's the order of configuration input for ruff? Is it project-config -> CLI arguments? Because in that case pylsp needs to at least be able to recognize that a [tools.ruff] table is present, otherwise it might overwrite it with the config that you pass when starting e.g. with neovim

@ccordoba12
Copy link
Member

Do you have any idea if/why pylsp needs to read the per-project config @ccordoba12?

To load settings saved on it for other tools.

@charliermarsh
Copy link

Yeah, CLI arguments always have precedence over the pyproject.toml.

I think it should be fine to either: (1) let Ruff handle the pyproject.toml, and only pass CLI arguments that come directly from the LSP config (which could include config, allowing users to override); or (2) keep the current logic of letting the LSP find the pyproject.toml, pass it in as --config if the user hasn't provided a config override in the LSP settings.

@charliermarsh
Copy link

Created an issue to track at #2. Once resolved, we can probably go ahead and add a reference to this from the Ruff README too.

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