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

fixable groups #3863

Closed
ghost opened this issue Apr 3, 2023 · 7 comments · Fixed by #7769
Closed

fixable groups #3863

ghost opened this issue Apr 3, 2023 · 7 comments · Fixed by #7769
Labels
core Related to core functionality

Comments

@ghost
Copy link

ghost commented Apr 3, 2023

How about supporting different groups of fixables in the configuration file?
So you can e.g. add a group for safe fixes that you always want to be fixed automatically like e.g. sorting imports.
And another group for fixes you only want applied automatically when you are focusing on improving code quality.

CLI could look something like this, e.g. for a group called "safe" defined in the config:
ruff check --fix safe .

@charliermarsh charliermarsh added the core Related to core functionality label Apr 4, 2023
@evanrittenhouse
Copy link
Contributor

evanrittenhouse commented Apr 9, 2023

I can start looking into this - work's a bit busy, so if it's urgent I may ask to be unassigned.

As far as the .toml changes required, I'm thinking something like this.

# ...
[fixable_groups]
safe = ["T001", "T002", "T003"]
code_quality = ["T001", "T004", "T006", "invalid_rule]
# ...

Groups shouldn't be mutually exclusive. Usage could be something like:

ruff check --fix code_quality .

If a rule is invalid, like in the case of code_quality, we should simply print a warning for that rule and perform autofixes on the valid rules in the group.

Let me know if that sounds like a decent design - if so, I can get started on implementation.

E: We could also do --fix-group instead of --fix - I'm trying to get a handle on the CLI implementation, but tough so far. If --fix-group would be easier than overriding --fix, would be happy to do it that way instead.

@Avasam
Copy link

Avasam commented Apr 13, 2023

This could probably be achieved through different config files and extend if extend supported an array. (though you could just call ruff multiple times instead of having an "all" config).

base.ruff.toml

# ...

safe.ruff.toml

extend = "./base.ruff.toml"
extend-select = ["T001", "T002", "T003"]

code_quality.ruff.toml

extend = "./base.ruff.toml"
extend-select = ["T001", "T004", "T006"]

pyproject.toml (all)

[tool.ruff]
extend = ["./safe.ruff.toml", "./code_quality.ruff.toml"]

ruff check --config ./safe.ruff.toml
ruff check --config ./code_quality.ruff.toml
ruff check (defaults to pyproject.toml)

@ghost
Copy link
Author

ghost commented Apr 13, 2023

Hi everyone,

thanks for pointing out a way to do this right now, Avasam!
In the long run I'd prefer the solution suggested by evanrittenhouse, as it is a lot easier to understand und requires fewer files.
Thank you for considering to implement this!

@evanrittenhouse
Copy link
Contributor

evanrittenhouse commented Apr 14, 2023

Cool. I'll get to work on implementing this

@charliermarsh
Copy link
Member

@evanrittenhouse - Before we start on implementation, I think we should make sure that we're aligned on design -- I want to make sure you don't sink time into something only for us to run into disagreements on core API and such later on :)

In the proposal here, is the idea such that users are responsible for defining those fix groups? I had historically thought of fixable levels as core metadata on the rule itself, e.g., you could have rules that are considered very safe to fix, rules that are experimental, rules that are risky and so may change semantics. If that were the case, users wouldn't be required to define fix groups. Instead, we could just define them in Ruff, and then users could specify an autofix safety level when running with --fix. Do you think that would be too inflexible?

(I also want to \cc @MichaReiser who's been thinking about configuration broadly and may well disagree with what I've written here :))

@evanrittenhouse
Copy link
Contributor

In my opinion, I like the idea of user-defined groups somewhere down the line. Safety levels would certainly be a helpful piece of information for rules, and I think it's quite valuable to add, but I wonder if that alone could shoehorn users into checks they don't want/need. They'd also allow us to only compare rules on one dimension (safe, unsafe, etc.), though we could probably get around that by implementing something like rule "traits" instead (handles imports, handles trailing commas, etc.).

For example, you could gather all rules around imports and run them together, or @xyxz-web's suggestion around "code quality" rules which could vary on a per-user basis.

Very open to more discussion on the design though! I haven't been around as long as some other folks so I definitely could be missing some stuff.

@ghost
Copy link
Author

ghost commented Apr 14, 2023

Personally I'd prefer to have the flexibility to define my own groups, though having a way to figure out which fixes are considered safe would also be very helpful.

zanieb added a commit that referenced this issue Oct 6, 2023
Rebase of #5119 authored by
@evanrittenhouse with additional refinements.

## Changes

- Adds `--unsafe-fixes` / `--no-unsafe-fixes` flags to `ruff check`
- Violations with unsafe fixes are not shown as fixable unless opted-in
- Fix applicability is respected now
    - `Applicability::Never` fixes are no longer applied
    - `Applicability::Sometimes` fixes require opt-in
    - `Applicability::Always` fixes are unchanged
- Hints for availability of `--unsafe-fixes` added to `ruff check`
output

## Examples

Check hints at hidden unsafe fixes
```
❯ ruff check example.py --no-cache --select F601,W292
example.py:1:14: F601 Dictionary key literal `'a'` repeated
example.py:2:15: W292 [*] No newline at end of file
Found 2 errors.
[*] 1 fixable with the `--fix` option (1 hidden fix can be enabled with the `--unsafe-fixes` option).
```

We could add an indicator for which violations have hidden fixes in the
future.

Check treats unsafe fixes as applicable with opt-in
```
❯ ruff check example.py --no-cache --select F601,W292 --unsafe-fixes
example.py:1:14: F601 [*] Dictionary key literal `'a'` repeated
example.py:2:15: W292 [*] No newline at end of file
Found 2 errors.
[*] 2 fixable with the --fix option.
```

Also can be enabled in the config file

```
❯ cat ruff.toml
unsafe-fixes = true
```

And opted-out per invocation

```
❯ ruff check example.py --no-cache --select F601,W292 --no-unsafe-fixes
example.py:1:14: F601 Dictionary key literal `'a'` repeated
example.py:2:15: W292 [*] No newline at end of file
Found 2 errors.
[*] 1 fixable with the `--fix` option (1 hidden fix can be enabled with the `--unsafe-fixes` option).
```

Diff does not include unsafe fixes
```
❯ ruff check example.py --no-cache --select F601,W292 --diff
--- example.py
+++ example.py
@@ -1,2 +1,2 @@
 x = {'a': 1, 'a': 1}
-print(('foo'))
+print(('foo'))
\ No newline at end of file

Would fix 1 error.
```

Unless there is opt-in
```
❯ ruff check example.py --no-cache --select F601,W292 --diff --unsafe-fixes
--- example.py
+++ example.py
@@ -1,2 +1,2 @@
-x = {'a': 1}
-print(('foo'))
+x = {'a': 1, 'a': 1}
+print(('foo'))
\ No newline at end of file

Would fix 2 errors.
```

#7790 will improve the diff
messages following this pull request

Similarly, `--fix` and `--fix-only` require the `--unsafe-fixes` flag to
apply unsafe fixes.

## Related

Replaces #5119
Closes #4185
Closes #7214
Closes #4845
Closes #3863
Addresses #6835
Addresses #7019
Needs follow-up #6962
Needs follow-up #4845
Needs follow-up #7436
Needs follow-up #7025
Needs follow-up #6434
Follow-up #7790 
Follow-up #7792

---------

Co-authored-by: Evan Rittenhouse <evanrittenhouse@gmail.com>
konstin pushed a commit that referenced this issue Oct 11, 2023
Rebase of #5119 authored by
@evanrittenhouse with additional refinements.

## Changes

- Adds `--unsafe-fixes` / `--no-unsafe-fixes` flags to `ruff check`
- Violations with unsafe fixes are not shown as fixable unless opted-in
- Fix applicability is respected now
    - `Applicability::Never` fixes are no longer applied
    - `Applicability::Sometimes` fixes require opt-in
    - `Applicability::Always` fixes are unchanged
- Hints for availability of `--unsafe-fixes` added to `ruff check`
output

## Examples

Check hints at hidden unsafe fixes
```
❯ ruff check example.py --no-cache --select F601,W292
example.py:1:14: F601 Dictionary key literal `'a'` repeated
example.py:2:15: W292 [*] No newline at end of file
Found 2 errors.
[*] 1 fixable with the `--fix` option (1 hidden fix can be enabled with the `--unsafe-fixes` option).
```

We could add an indicator for which violations have hidden fixes in the
future.

Check treats unsafe fixes as applicable with opt-in
```
❯ ruff check example.py --no-cache --select F601,W292 --unsafe-fixes
example.py:1:14: F601 [*] Dictionary key literal `'a'` repeated
example.py:2:15: W292 [*] No newline at end of file
Found 2 errors.
[*] 2 fixable with the --fix option.
```

Also can be enabled in the config file

```
❯ cat ruff.toml
unsafe-fixes = true
```

And opted-out per invocation

```
❯ ruff check example.py --no-cache --select F601,W292 --no-unsafe-fixes
example.py:1:14: F601 Dictionary key literal `'a'` repeated
example.py:2:15: W292 [*] No newline at end of file
Found 2 errors.
[*] 1 fixable with the `--fix` option (1 hidden fix can be enabled with the `--unsafe-fixes` option).
```

Diff does not include unsafe fixes
```
❯ ruff check example.py --no-cache --select F601,W292 --diff
--- example.py
+++ example.py
@@ -1,2 +1,2 @@
 x = {'a': 1, 'a': 1}
-print(('foo'))
+print(('foo'))
\ No newline at end of file

Would fix 1 error.
```

Unless there is opt-in
```
❯ ruff check example.py --no-cache --select F601,W292 --diff --unsafe-fixes
--- example.py
+++ example.py
@@ -1,2 +1,2 @@
-x = {'a': 1}
-print(('foo'))
+x = {'a': 1, 'a': 1}
+print(('foo'))
\ No newline at end of file

Would fix 2 errors.
```

#7790 will improve the diff
messages following this pull request

Similarly, `--fix` and `--fix-only` require the `--unsafe-fixes` flag to
apply unsafe fixes.

## Related

Replaces #5119
Closes #4185
Closes #7214
Closes #4845
Closes #3863
Addresses #6835
Addresses #7019
Needs follow-up #6962
Needs follow-up #4845
Needs follow-up #7436
Needs follow-up #7025
Needs follow-up #6434
Follow-up #7790 
Follow-up #7792

---------

Co-authored-by: Evan Rittenhouse <evanrittenhouse@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Related to core functionality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants