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

Disable autofix by default #1657

Merged
merged 1 commit into from
Dec 14, 2023
Merged

Disable autofix by default #1657

merged 1 commit into from
Dec 14, 2023

Conversation

SuperAuguste
Copy link
Member

@SuperAuguste SuperAuguste commented Dec 8, 2023

Closes #1623

@SuperAuguste SuperAuguste marked this pull request as ready for review December 8, 2023 22:09
@SuperAuguste SuperAuguste force-pushed the auguste/disable-autofix branch from c286ef5 to 7b5e940 Compare December 8, 2023 22:09
Copy link

codecov bot commented Dec 8, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (128a161) 75.22% compared to head (f3fe901) 75.22%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1657   +/-   ##
=======================================
  Coverage   75.22%   75.22%           
=======================================
  Files          33       33           
  Lines        9054     9054           
=======================================
  Hits         6811     6811           
  Misses       2243     2243           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@xdBronch
Copy link
Contributor

xdBronch commented Dec 8, 2023

i feel that most of the dislike of autofix was because of the var -> const one (which is already disabled now). i dont think ive seen many if any people complain about the discarding unused variables which ime helps a lot of people get over the unused variable error

@SuperAuguste
Copy link
Member Author

I've seen people complain about that unfortunately. I've just had enough of those comments so I'm going to merge this.

@SuperAuguste SuperAuguste force-pushed the auguste/disable-autofix branch from 7b5e940 to f3fe901 Compare December 8, 2023 22:20
@Techatrix
Copy link
Member

I've just had enough of those comments so I'm going to merge this.

Have you tried ignoring people that have a passion for complaining? =^_^=

@SuperAuguste
Copy link
Member Author

It's hard to ignore them when even your friends start saying the same stuff (albeit in a much nicer way). And they do have a point: editing people's code by default is a little jarring, I've experienced (and hated) this myself with the Go LSP, so I do get where they're coming from.

@Techatrix
Copy link
Member

Techatrix commented Dec 8, 2023

And they do have a point: editing people's code by default is a little jarring

That is a valid argument. That is something I would have liked to discuss in #1623.

Let me make a counterargument:
Autofix is not the only that is editing people's code.
The vscode-zig has format on save enabled by default which beyond formatting, upgrades your code by applying fixups.
Of course, autofix and fixups are not the same, but they both editing people's code without permission, so is the editing of people's code that is the problem or is there an underlying issue? Perhaps they perceive the actions of autofix to be just undesired, not self-explaining or just confusing?

@GrayHatter
Copy link

Let me make a counterargument:
Autofix is not the only that is editing people's code. The [code that's not zls] has [...]

Just because other projects are doing something is hardly related to if zls should or shouldn't do it. Useful to study and figure out sure. But definitely not something to emulate! Good software will default having fewer opinions whenever possible. Formatting/style is opinionated by definition, and IIRC Zig formatting used to be required for the compiler to function. Consistent formatting is useful for source control features, and, the point of installing tooling. If I wanted to format my code myself, I wouldn't install a code formatter. But I didn't install ZLS because I wanted it to fix or format my code, I installed it to get better linting hints, and better syntax highlighting. I never wanted it to change my code.

The problem for me isn't which fixup's it's making, the problem is that it's making decisions about my code without asking first. I think autofix is a great feature, and one zls should have. But it's not one I'd ever use, never for any of my code. It's not just jarring, it also breaks how I interact with code, unused variable warnings are compile errors for a reason, and I appreciate them! The author needs to make a decision, use the variable, or intentionally suppress it. Having my LSP do it for me, especially when I didn't want it, and when I didn't ask for it, is problematic. If autofix is happy to suppress these, errors why wouldn't it add @constCast for me?

Autofix shouldn't be enabled by default, because I don't trust autofix to act reasonably, if I want to take on the risk that something changed my code and I missed a bug because of it, that's my decision to make, but I should have to make the decision to enable it.

I'm currently compiling from source myself because this is important to me, it's also mentioned as a warning every time I mention or suggest zls.

Unsolicited rant aside, I only found out about this pull because I noticed the branch was added when I pulled to check for updates. So I just came by to say thank you @SuperAuguste, I really appreciate this change!

@llogick llogick merged commit 9476a1d into master Dec 14, 2023
10 checks passed
@llogick llogick deleted the auguste/disable-autofix branch December 14, 2023 15:11
Techatrix pushed a commit that referenced this pull request Dec 19, 2023
@Narsil
Copy link

Narsil commented May 24, 2024

@SuperAuguste

Random comment of sympathy about the autofix feature.

Having it enabled by default was super nice for me as a beginner (when iterating on code really quick it's quite annoying when something doesn't build because I quickly uncommented part of the logic).

It's ok if it's not the default (I can sort of get the rationale), I enabled it for me since I highly value the speedup to the devtime when changing stuff around a lot.

Thanks a lot for the feature, and thanks for making it the default for a bit, I wouldn't have discovered it without it, and it definitely eases the pain of zig complaining about unused variables (which again, I appreciate, but it's annoying when you're not trying to clean up your code, just iterating fast on some parts of your code while investigating something)

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.

Should autofix be disabled by default?
6 participants