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

Print only file path when reporting a correction #5596

Merged
merged 1 commit into from
Jun 28, 2024

Conversation

SimplyDanny
Copy link
Collaborator

Omit exact line and column information. Keep the internal logic available for a while in case people complain for good reasons. Then the change can be reverted easily.

Reasons behind this change:

  • What is the position of a correction actually? The position where the rule triggered? The position of the transformed node?
  • If there's more than a single fix, it's highly likely that all positions (except for the first) are wrong no matter what the answer to the first question is.
  • Getting the positions right in a rewriter is hard. And there is always the feeling that something is wrong with them - probably because it is.
  • Even if you get the positions right in a single rule: One run collects positions for all rules that apply to a file and so rewrites in other rules might draw existing positions wrong.
  • We already have checks for more than one correction position disabled in tests due to these issues.

@SwiftLintBot
Copy link

SwiftLintBot commented May 18, 2024

17 Messages
📖 Linting Aerial with this PR took 0.8s vs 0.81s on main (1% faster)
📖 Linting Alamofire with this PR took 1.13s vs 1.14s on main (0% faster)
📖 Linting Brave with this PR took 6.66s vs 6.62s on main (0% slower)
📖 Linting DuckDuckGo with this PR took 3.51s vs 3.49s on main (0% slower)
📖 Linting Firefox with this PR took 9.79s vs 9.86s on main (0% faster)
📖 Linting Kickstarter with this PR took 8.21s vs 8.16s on main (0% slower)
📖 Linting Moya with this PR took 0.49s vs 0.48s on main (2% slower)
📖 Linting NetNewsWire with this PR took 2.26s vs 2.24s on main (0% slower)
📖 Linting Nimble with this PR took 0.68s vs 0.68s on main (0% slower)
📖 Linting PocketCasts with this PR took 7.05s vs 7.11s on main (0% faster)
📖 Linting Quick with this PR took 0.38s vs 0.37s on main (2% slower)
📖 Linting Realm with this PR took 4.32s vs 4.3s on main (0% slower)
📖 Linting Sourcery with this PR took 2.08s vs 2.07s on main (0% slower)
📖 Linting Swift with this PR took 4.02s vs 4.01s on main (0% slower)
📖 Linting VLC with this PR took 1.13s vs 1.11s on main (1% slower)
📖 Linting Wire with this PR took 15.12s vs 15.06s on main (0% slower)
📖 Linting WordPress with this PR took 11.59s vs 11.53s on main (0% slower)

Generated by 🚫 Danger

Omit exact line and column information. Keep the internal logic
available for a while in case people complain for good reasons. Then
the change can be reverted easily.

Reasons behind this change:

* What is the position of a correction actually? The position where the rule triggered? The position of the transformed node?
* If there's more than a single fix, it's highly likely that all positions (except for the first) are wrong no matter what the answer to the first question is.
* Getting the positions right in a rewriter is hard. And there is always the feeling that something is wrong with them - probably because it is.
* Even if you get the positions right in a single rule: One run collects positions for all rules that apply to a file and so rewrites in other rules might draw existing positions wrong.
* We already have checks for more than one correction position disabled in tests due to these issues.
@mildm8nnered
Copy link
Collaborator

I think if the information we're providing now is misleading, then suppressing it is ok.

The list of problems above makes me think that maybe we're doing something wrong here - they all sound fixable - at the end of the day it's just a set of transformations on a set of files.

My personal use-case for autocorrect is that we run it during every build for whitespace correction, colon spacing etc, so no-one ever looks at the actual violation positions, and we never enable autocorrect for new rules - we just fix them manually, or add them to our baseline - not for any principled reason - we just never have.

I guess the other use case is having a legacy code-base and doing a huge cleanup sweep - in theory that should work, but personally I'd be a little bit wary of turning on fix for everything and letting rip.

If I did care about fix violation positions, and wanted to see them in Xcode for example, I think we could come up with accurate violation positions, taking into account all the other fixes, but we would need to take into account all the other fixes, and that sounds like a lot of work for something that I'm not sure many people would ever look at.

@SimplyDanny
Copy link
Collaborator Author

Yes, there are potential ways to fix this and get it right. Yet, they don't seem to be easy. We will see if anyone is relying on these positions. I actually don't think so given that these locations are just wrong most of the time. For the moment, it's better to have a simpler output that is correct.

@SimplyDanny SimplyDanny merged commit 09edd52 into realm:main Jun 28, 2024
12 checks passed
@SimplyDanny SimplyDanny deleted the correction-positions branch June 28, 2024 05:27
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.

None yet

3 participants