-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Update: "off" options for "space-before-blocks" (refs #10906) #10907
Update: "off" options for "space-before-blocks" (refs #10906) #10907
Conversation
7a3d805
to
2c9f322
Compare
See eslint#10906 Where "always" enforces space and "never" enforces no-space, "off" does not enforce any space preference. It may be useful to add a similar "off" option to other enums only allowing "always" and "never", too, but that is outside the scope of this PR. TODO: Why do two tests fail? (what the heck???) TODO: The documentation will need to describe this addition in more detail.
2c9f322
to
ea987a3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've identified the test failure issues. Please make the suggested changes and hopefully your tests will pass.
I somehow managed to insert "No" into totally the wrong place in that last commit...
Me and the commit-message bot are not getting along very well It would be really nice and helpful if when I clicked "details" it could tell me specifically what was wrong with the title or commit message instead of just screaming a red "X" and "RTFM" at me |
Can you please also update this PR description using the template specified? |
@nzakas I made the requested changes. |
Thanks @pineapplemachine for updating the initial post. I'll champion this. FYI: We won't be able to merge this in until the proposal you laid out in your original message is accepted. As champion, it's my responsibility to obtain support from the team, although you are welcome to help keep discussion going as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks! (Sorry for the delay in coming back to this.) Personally, I'm okay with the documentation changes as they are, but maybe other team members will have some suggestions.
FYI, the PR title had a trailing space, which was upsetting our commit-message bot. I've fixed it. |
@eslint/eslint-team Please take a look at the proposal here and (hopefully) give it a 👍. Thanks! |
Thanks @pineapplemachine! |
What is the purpose of this pull request? (put an "X" next to item)
[X] Changes an existing rule (template)
What rule do you want to change?
This PR modifies the behavior of the
space-before-blocks
rule.Does this change cause the rule to produce more or fewer warnings?
This PR does not affect the number of warnings for existing configurations of the
space-before-blocks
rule. It allows a new configuration option which ignores whitespace for functions, classes, and/or keywords, no longer warning regardless of whether whitespace was included in the specified cases.How will the change be implemented? (New option, new default behavior, etc.)?
The PR adds a new accepted value to existing options. Where previously only
"always"
and"never"
were accepted, a third option"off"
is now also accepted."off"
means that neither style of whitespace should be enforced for a certain category of blocks. Please refer to the diff for details.Please provide some example code that this change will affect:
What does the rule currently do for this code?
The
space-before-blocks
rule enforces that there is either"always"
or"never"
whitespace between the opening brace '{' and the previous non-whitespace character, with the option to have separate enforcement for functions, classes, and keywords.What will the rule do after it's changed?
The rule is able to enforce
"always"
and/or"never"
rules for one or more of functions, classes, and keywords, and not enforce either style for the remaining cases. This is not possible with the current behavior and options.More info:
Please see this issue: #10906
The behavior of the "space-before-blocks" is affected. New functionality is added. All prior tests still pass without changes, which implies that the changes are fully backwards-compatible with prior eslint configurations.
Where "always" enforces space and "never" enforces no-space, "off" does not enforce any space preference. This can be useful, for example, when enforcing one whitespace setting for "functions" but not enforcing any setting for other kinds of blocks.
It may be useful to add a similar "off" option to other rules currently only accepting "always" and "never", too, but that is outside the scope of this PR. (This is the only such change that would have an impact on my own usage of eslint.)
TODO:
I added to the documentation, but not in as much detail as is probably warranted. Since I don't know that something like this exists elsewhere in the docs, I was unsure with how to proceed and describe the feature in more detail. I would really appreciate some input here.
[DONE] Two tests fail and I do not understand why. I don't think this is related to the implementation changes; I think that I have misunderstood something in regards to writing tests which use the "class" keyword. (Why do the tests on L457 and L464 pass but two of the tests that I added do not??)