Skip to content
This repository has been archived by the owner on Aug 12, 2023. It is now read-only.

feat(ruff): add support for ruff python linter #1068

Merged
merged 7 commits into from
Nov 7, 2022

Conversation

vsedov
Copy link
Contributor

@vsedov vsedov commented Sep 5, 2022

No description provided.

@vsedov
Copy link
Contributor Author

vsedov commented Sep 5, 2022

Quick note: I wasn't sure how to deal with severity, and currently only works when you have a project.toml, I il try working around this.

@jose-elias-alvarez
Copy link
Owner

Requiring a config file isn't a big deal as long as we mention it in the documentation, which we can do using the notes field (see this example). The exit code format looks similar to flake8, so perhaps we could use that as an example.

Could you also take a look at this comment on another PR about stdin? I think we'll run into similar issues here.

@jose-elias-alvarez
Copy link
Owner

Exit code handling looks good! I poked around the repository and wasn't able to find any information about stdin support, so I think using a temp file will provide the best experience (though it may run into issues resolving config files until #1075 is merged).

@vsedov
Copy link
Contributor Author

vsedov commented Sep 8, 2022

Is there a good example for using a temp file ? Im guessing something like this Right ?
Il try messing around with temp files a bit more.

Cheers.

@jose-elias-alvarez
Copy link
Owner

Right, setting to_temp_file = true should make it work with no additional code. I merged #1075, so I think everything should work as expected, but let me know.

@vsedov
Copy link
Contributor Author

vsedov commented Sep 9, 2022

That should be it, didn't come across any major errors only thing that confused me was in poetry projects it required the root dir, but as you stated, pretty certain that can be updated in the docs.

@jose-elias-alvarez
Copy link
Owner

Great, this looks good to go! If we can just add a mention of any setup requirements to the notes field, I think this is ready to merge (more info here).

@YangtseSu
Copy link

YangtseSu commented Oct 12, 2022

  1. Support linting input from stdin
    We can use --stdin-filename now.
  2. ruff raise an error when No pyproject.toml found, null-ls did not pass the error message to on_output in this case.
ruff ./E402.py
No pyproject.toml found.
Falling back to default configuration...
Found 11 error(s).
E402.py:6:1: F401 `a` imported but unused
E402.py:9:5: F401 `b` imported but unused
E402.py:15:1: F401 `c` imported but unused
E402.py:17:4: F821 Undefined name `x`
E402.py:18:5: F401 `d` imported but unused
E402.py:20:5: F401 `e` imported but unused
E402.py:22:5: F821 Undefined name `x`
E402.py:24:1: E402 Module level import not at top of file
E402.py:24:1: F401 `f` imported but unused
E402.py:28:5: F401 `e` imported but unused
E402.py:32:5: F401 `g` imported but unused
8 potentially fixable with the --fix option.

when ignore_stderr = true ,it works for python single file.
when from_stderr = true, it works for python project with pyproject.toml.
3. The serverities information can be found in https://github.com/charliermarsh/ruff#rules

@YangtseSu
Copy link

YangtseSu commented Oct 12, 2022

I tried to fix this.
This file works under all the conditions I have tested.
But I don't know how to submit a commit to this PR.
ruff.lua

@vsedov
Copy link
Contributor Author

vsedov commented Oct 28, 2022

I tried to fix this. This file works under all the conditions I have tested. But I don't know how to submit a commit to this PR. ruff.lua

Hi il add it now, I'm very sorry I have not been working on this, I've been focusing on uni so my time has become very niche, it should be up now :)

Thanks for helping out as well >.<

@vsedov
Copy link
Contributor Author

vsedov commented Oct 28, 2022

Think this is good to merge? @jose-elias-alvarez Did we miss anything?

@haoyun
Copy link

haoyun commented Nov 2, 2022

Think this is good to merge? @jose-elias-alvarez Did we miss anything?

I would like to see this to be merged. However, as mention above,

If we can just add a mention of any setup requirements to the notes field, I think this is ready to merge

@YangtseSu
Copy link

Think this is good to merge? @jose-elias-alvarez Did we miss anything?

I would like to see this to be merged. However, as mention above,

If we can just add a mention of any setup requirements to the notes field, I think this is ready to merge

Install ruff and enable ruff diagnostics. Nothing special needs to be added to "notes" currently.

@jose-elias-alvarez
Copy link
Owner

Think this is good to merge? @jose-elias-alvarez Did we miss anything?

I would like to see this to be merged. However, as mention above,

If we can just add a mention of any setup requirements to the notes field, I think this is ready to merge

Install ruff and enable ruff diagnostics. Nothing special needs to be added to "notes" currently.

My understanding was that this source only works if you have a project.toml file. If that's the case, this should be mentioned in the documentation. Ideally we could also catch errors stemming from missing config files and show them as diagnostics, as in this example, but we can save that for a follow-up.

If that's not the case and the linter runs properly without any additional setup, then this is good to go, so let me know.

@YangtseSu
Copy link

The linter runs properly without any additional setup, now.

@YangtseSu
Copy link

There are two things that may need to be resolved in the future. One is that the upstream does not distinguish between errors, warnings and information, but outputs error information according to the function. The severity is not absolutely correct.ruff is in very active development, and new error types may appear.

The second is that ruff has the - fix option, maybe it is possible to add code_actions.

@jose-elias-alvarez
Copy link
Owner

Great, thanks for confirming and thanks everyone for the work on this!

@jose-elias-alvarez jose-elias-alvarez merged commit 7cbdab6 into jose-elias-alvarez:main Nov 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants