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

Avoiding passing pyproject.toml options as command-line arguments #2

Closed
charliermarsh opened this issue Dec 10, 2022 · 7 comments · Fixed by #4
Closed

Avoiding passing pyproject.toml options as command-line arguments #2

charliermarsh opened this issue Dec 10, 2022 · 7 comments · Fixed by #4

Comments

@charliermarsh
Copy link

charliermarsh commented Dec 10, 2022

See discussion in #1.

@charliermarsh
Copy link
Author

\cc @jhossbach

@jhossbach
Copy link
Member

jhossbach commented Dec 10, 2022

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.

I'd rather go with (1), it feels cleaner to let ruff handle the project-wide settings.

Shouldn't the configuration hierarchy be project config > pylsp config? Say e.g. you disagree with the default line length of 88, so instead you configure your lsp via neovim to be 100 by default. If you now work in a project with a pyproject.toml, you would expect the LSP to use the configuration there instead, or should it always take the user-configured line-length despite any pyproject.toml file present?

We could add some logic to use the user-configured options only if the pyproject.toml file doesn't contain this option.

@charliermarsh
Copy link
Author

Yeah I think you're right that project configuration should take precedence. I don't really have a great answer for how we should enable "defaults" in those other cases. 🤔

@jhossbach
Copy link
Member

jhossbach commented Dec 11, 2022

This should work:

for key, value in toml_config.items():
    if key in [
        "exclude",
        "ignore",
        "line-length",
        "per-file-ignores",
        "select",
    ]:
        settings[key] = value

It will skip any entries in the pyproject.toml that are not in the list. We can extend this list if we want, but additional arguments in the toml file are used automatically by ruff anyway so unless someone wants to have more configurability from within pylsp I don't think this is necessary.

@charliermarsh
Copy link
Author

Ruff is likely to support extending pyproject files (i.e., you could point to another pyproject file, and it’ll automatically merge in its settings). Which could make this hard to maintain, since you’d miss extended properties (or have to resolve the extension).

I think the safest thing to do would be: look for a pyproject.toml in all ancestor directories of the file. If we find one, don’t apply any of the LSP settings as command-line arguments (defer to Ruff). If we don’t, then apply the LSP settings as command-line arguments. What do you think?

@jhossbach
Copy link
Member

I think this makes sense. In case I was working in a project with some configuration for ruff, I would expect that the default for ruff should still apply even if the option is not specified, so even if e.g. line-length was not explicitly set to 88 it would still apply within the project. I will create a PR for this.

@charliermarsh
Copy link
Author

Awesome, happy to review!

jhossbach added a commit that referenced this issue Dec 11, 2022
Sets config options (except `executable`) to `None` if `pyproject.toml`
file is detected.
jhossbach added a commit that referenced this issue Dec 11, 2022
Sets config options (except `executable`) to `None` if `pyproject.toml`
file is detected.
jhossbach added a commit that referenced this issue Dec 11, 2022
Set ruff config to empty dict if pyproject.toml is present
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 a pull request may close this issue.

2 participants