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

Ecosystem check: Set --no-fix #15146

Open
MichaReiser opened this issue Dec 26, 2024 · 4 comments
Open

Ecosystem check: Set --no-fix #15146

MichaReiser opened this issue Dec 26, 2024 · 4 comments
Assignees
Labels
ci Related to internal CI tooling help wanted Contributions especially welcome

Comments

@MichaReiser
Copy link
Member

          > It looks like in the ecosystem check a bunch of violations were removed with no fix, so I'm wondering if a syntax error was introduced by the fix. Maybe when there is a default argument something goes wrong?

I think this happens for projects where the ruff.toml or pyproject.toml contains fix = true. I also got tripped up by this behavior in some of my pull requests. I think it would be a good idea to change the ecosystem script to pass --no-fix, so this doesn't happen, since the output will always be interpreted incorrectly when fixes have been applied (this can also lead to other confusing things like new E501 flags, because the fix caused a line to get too long, or an unrelated unfixable violation getting flagged, because the fix caused the violation to be slightly offset, so you see a +1 -1 for that violation).

Originally posted by @Daverball in #15139 (comment)

@MichaReiser MichaReiser added help wanted Contributions especially welcome ci Related to internal CI tooling labels Dec 26, 2024
@dylwil3
Copy link
Collaborator

dylwil3 commented Dec 26, 2024

Wouldn't this remove valuable information about fixes that we get from the ecosystem check?

Maybe we should just run it both with and without fixes and use the former to populate the +/- violations and the latter for +/- fixes?

@MichaReiser
Copy link
Member Author

Wouldn't this remove valuable information about fixes that we get from the ecosystem check?

It helps showing how some fixes enable other fixes but I've found the result more confusing than useful. To the point where differences where often ignored because "the ecosystem check is weird, that must be a false positive).

We could consider having a second ecosystem check that always runs with --fix if we have specific use cases where we find this useful.

@Daverball
Copy link
Contributor

I agree that having some output about applied fixes would be useful to have in the ecosystem results (especially in order to catch fixes that are unstable in some scenarios, that weren't covered by the tests), but it needs to be deliberate, with its own section in the results table, rather than incidental, so the results can be interpreted correctly.

Right now for a new rule with a fix that either always works or works most of the time we will generally see a lower ecosystem impact than the rule actually has, to an unpredictable degree based on the individual project configurations.

@eclbg
Copy link

eclbg commented Jan 3, 2025

Hey! I'm working on this and will submit a PR shortly. Feel free to assign it to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci Related to internal CI tooling help wanted Contributions especially welcome
Projects
None yet
Development

No branches or pull requests

4 participants