-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Remove prefer-const
as it breaks code if standard --fix
is used for autoformatting
#1765
Comments
In my personal opinion: This same argument can be applied to lots of in-progress code. Comment out an A linter as I see it is supposed to protect and improve the final product, not the in-progress code. If one eg. have an auto-fix on save, then the easiest fix is to remove that auto-fix and instead do manual fixing. I personally never run any automated fixing and often opt to fix things manually, by eg using the hover boxes that appear on lint errors in VS Code. |
Hi @voxpelli,
Sure, and as noted I think a complaint (warning) would be correct here. The current implementation silently changes the code, potentially into a broken state.
Um. That's reasonable as a preference, but what you're saying here implies that standard's autoformat is a potentially destructive operation, and the developer needs to understand when it is and isn't safe to do. That's not how autoformat works in any other tool I've used, surely it's not the intent here? |
@voxpelli Sorry, so the answer here is that you agree autoformat is breaking code in the listed cases, and it's not a bug because your preference is to not autoformat in those cases? |
@fenomas I can see the issue with this rule when it comes to autofixing on save, but I'm not convinced that is necessarily an issue with If we see it as a design goal that Designing How have you set up your autofix on save? Using the You could set up your own |
Rule in question: To note is also that there's not really any way of having a rule active in ESLint, but disabling the autofix for it. So you would have to make it on the call to the fix: eslint somedir --fix --rule "somerule: 0" Whether something like that would be possible to add to eg. the |
@voxpelli This is a confusing answer, as autofix-on-save appears to be a configurable option in all of the official editor plugins. All of their docs have a note about how to enable it, with no warnings - even the readme of this repo has a note about it. It looks like a regular bread-and-butter feature, not an unsupported (much less unsupportable) edge case. With that said though, personally I don't use fix-on-save, and I don't think it's directly related to the issue. The high-order bit is, autoformat is normally a "safe", non-destructive operation. Nobody assumes that it will always fix all errors, but people do assume that it won't change working code into a non-working state. So personally, I use it more frequently than I save - whenever something needs to be formatted, I hit autoformat. And I can't recall ever having seen an autoformatter that wasn't safe by default - not with standard (outside of this issue), prettier, eslint, vscode's default formatter, etc. |
Just to be clear: We use the formatting behavior of ESLint. Keep in mind that If you really want this behavior, you could open an issue in the ESLint repo.
|
I have yet to se it demonstrated that it turns any code into a non-working state though? |
As far as I know eslint's formatter is safe by default. The rule we're talking about is not enabled by
I don't know what that means. When is code ever "final"? If you put a note in the readme saying "warning: do not run As in the original issue, the code doesn't break immediately but will break when the reassignable variable gets reassigned. E.g. when code that was temporarily commented out gets uncommented, etc. |
A linter looks at the piece of code as is. It doesn't assume that any code has been commented out or should be expected to be added. It looks at the code as is and sees if it fulfils the requirements that has been set up.
That "can" is determined by looking at the state of the project as its linted. If you run Edit: Also, bear in mind that we are many co-maintainers on this project and not all of us are working on all of the tools, I myself have worked on none of the autofix stuff, hence why my response there may have sounded confusing. |
I fully agree, but that's the opposite of what's been said so far. If the linter requires code to be "final" (still no idea what that means), then it's obviously making assumptions about what future edits will (not) happen. If it takes code as-is, then it shouldn't assume anything about the state of the code. I mean, consider the error where a variable is declared but never used. If the code is "final", then it's safe to fix that error by removing the variable entirely. But you wouldn't want your formatter to remove all unused variables silently by default, right? I'm saying the same logic should apply here. PS: is this "linters are only supposed to work with final code" idea discussed anywhere I could look at? I've used eslint and other linters pretty extensively and I've never heard anything at all similar. |
Hi @fenomas the |
@dougwilson Of course I'm aware that
It's standard that's choosing which fixable rules to apply. ESlint has a number of rules that are fixable, but which standard does not autofix by default - I'm suggesting that |
So just to clarify: are you saying that standard should never make suggestions for if vars should be |
But if that's not possible, and the only options are both warning+autofix, or neither, then neither would seem preferable. At the end of the day, there are situations where an author declares a |
Right, that is what I'm trying to say: that a warning without autofix is not possible, at least not working you opening a feature request for that in eslint, as eslint doesn't offer such a configuration for this. That is why the above was pointing you to eslint, if that makes sense. |
Thanks for clarifying that @dougwilson. In that case, per my last comment I think the right thing to do here is for standard to not enable PS: it would be helpful if you could shed any light on the "linters are only for the final product, not the in-progress code" policy quoted extensively above. I've looked around various eslint docs, and the policy doesn't appear to exist outside this thread. |
I am not familiar with that policy, and unfortunately cannot shed any light on it. |
I only ask because (a) the policy appears to not exist, and (b) it's the only reason given so far for why this issue is closed. Honestly, I'm not trying to be a weird hardass here - if the answer to this issue is "we agree there are potential risks but standard is opinionated and our opinion is to keep the current behavior", then that's 100% a reasonable answer. But currently the only answer (from @voxpelli) is: "fixing on save isn't supported because linters are only meant for final code". And it appears that neither standard nor eslint actually has or follows that policy/design/intent. |
Hi @fenomas apologies on not being able to elaborate; I am actually just a user of standard (and so somewhate vested in what the rules are) but not technically part of the project as a maintainer or anything. Apologies if I caused any confusion on that. You can typically see what role different commenters have based on the badge github places on the comments, though. |
@dougwilson Understood, and thanks for clarifying. (questions for maintainers about the "only final code" policy remain open.) |
There is no such "policy" and it's also not something where (As eg. pointed out, the decision to make So, no policy, just me trying to formulate my thoughts. If you want an explicit reason for the closing this then I would eventually close this due to:
Hence making this an issue with ESLint, not |
@voxpelli I'm not suggesting that ESLint does not apply the rule by default, so there is no issue to report to ESLint. Standard does apply it by default, so I'm reporting here. Thanks for understanding. |
Right, here's the discussions why it was added: |
prefer-const
as it breaks code in some situations
Thank you for the links - I searched issues, but I didn't think to search pull requests. |
I've just realized, maybe the disconnect here is whether The readme and other docs here describe it as an autoformatter, so that's what I've been assuming. And if you use But it sounds like maintainers here are using @voxpelli, is that latter usage what you meant about using |
Thanks for confirming @voxpelli - apologies if I sounded snippy before, but I was really confused why it would be necessary to finalize code before autoformatting. 😅 I'll edit the original issue to make things clearer. As for the issue itself, to be honest if OTOH if |
prefer-const
as it breaks code in some situationsprefer-const
as it breaks code if standard --fix
is used for autoformatting
What version of this package are you using?
standard@16.0.4
What operating system, Node.js, and npm version?
the online repl
What happened?
When using
standard --fix
as an autoformatter, theprefer-const
rule autofix breaks code if the developer autoformats frequently.This happens whenever a variable needs to be reassignable, but the code where it gets reassigned isn't present (e.g. because it's not written yet, temporarily commented out, etc.). In such cases, autoformatting with
standard --fix
will change the declaration to a const, which will break the code later when the variable reassignments are restored.Note: this issue only really applies if the developer uses
standard --fix
like an autoformatter, running it frequently as code is written. In the comments it became that several maintainers don't use--fix
this way, and see it as something to be done frequently, e.g. during code tests. However the readme and docs refer to--fix
as an auto formatter, so this issue is written with that assumption.What did you expect to happen?
The ideal case would be to show a linter warning for
let
variables that aren't reassigned, but not convert them toconst
during autoformat. From the comments it sounds like this isn't possible, so removingprefer-const
sounds like the only fix.Alternately, if
standard --fix
is not meant to be used frequently as an autoformatter, then there's no need to remove the rule (but in that case the docs should be changed to refer to the command some other way, and standard might want to choose an official preferred autoformatter).Are you willing to submit a pull request to fix this bug?
No.
Note: this issue is edited thanks to discussion w/ maintainers - please note that the top ~25 comments are about the pre-edit version.
The text was updated successfully, but these errors were encountered: