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

Update: "off" options for "space-before-blocks" (refs #10906) #10907

Merged
merged 2 commits into from Nov 9, 2018

Conversation

@pineapplemachine
Copy link
Contributor

commented Oct 1, 2018

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:

function MyFunction() { ... }
class MyClass { ... }
if( ... ) { ... }

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??)

  18549 passing (48s)
  2 failing

  1) space-before-blocks
       invalid
         class test { constructor(){} }:

      AssertionError [ERR_ASSERTION]: A fatal parsing error occurred: Parsing error: The keyword 'class' is reserved
      + expected - actual

      -false
      +true

      at testInvalidTemplate (lib/testers/rule-tester.js:9:13617)
      at Context.RuleTester.it (lib/testers/rule-tester.js:9:20143)

  2) space-before-blocks
       invalid
         class test { constructor() {} }:

      AssertionError [ERR_ASSERTION]: A fatal parsing error occurred: Parsing error: The keyword 'class' is reserved
      + expected - actual

      -false
      +true

      at testInvalidTemplate (lib/testers/rule-tester.js:9:13617)
      at Context.RuleTester.it (lib/testers/rule-tester.js:9:20143)
@jsf-clabot

This comment has been minimized.

Copy link

commented Oct 1, 2018

CLA assistant check
All committers have signed the CLA.

@eslint eslint bot added the triage label Oct 1, 2018

@pineapplemachine pineapplemachine force-pushed the pineapplemachine:sophie/github-issue-10906 branch from 7a3d805 to 2c9f322 Oct 1, 2018

@pineapplemachine pineapplemachine changed the title Add "off" options for "space-before-blocks" New: "off" options for "space-before-blocks" (refs #10906) Oct 1, 2018

Update: "off" options for "space-before-blocks" (refs #10906)
See #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.

@pineapplemachine pineapplemachine force-pushed the pineapplemachine:sophie/github-issue-10906 branch from 2c9f322 to ea987a3 Oct 1, 2018

@pineapplemachine pineapplemachine changed the title New: "off" options for "space-before-blocks" (refs #10906) Update: "off" options for "space-before-blocks" (refs #10906) Oct 1, 2018

@platinumazure
Copy link
Member

left a comment

I've identified the test failure issues. Please make the suggested changes and hopefully your tests will pass.

tests/lib/rules/space-before-blocks.js Outdated Show resolved Hide resolved
tests/lib/rules/space-before-blocks.js Outdated Show resolved Hide resolved
Fix: Tests pass for space-before-blocks rule change (refs #10907)
I somehow managed to insert "No" into totally the wrong place in that last commit...
@pineapplemachine

This comment has been minimized.

Copy link
Contributor Author

commented Oct 2, 2018

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

@nzakas

This comment has been minimized.

Copy link
Member

commented Oct 4, 2018

Can you please also update this PR description using the template specified?

@pineapplemachine

This comment has been minimized.

Copy link
Contributor Author

commented Oct 5, 2018

@nzakas I made the requested changes.

@platinumazure platinumazure self-assigned this Oct 13, 2018

@platinumazure

This comment has been minimized.

Copy link
Member

commented Oct 13, 2018

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.

@platinumazure
Copy link
Member

left a comment

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.

@platinumazure platinumazure changed the title Update: "off" options for "space-before-blocks" (refs #10906) Update: "off" options for "space-before-blocks" (refs #10906) Oct 13, 2018

@platinumazure

This comment has been minimized.

Copy link
Member

commented Oct 13, 2018

FYI, the PR title had a trailing space, which was upsetting our commit-message bot. I've fixed it.

@platinumazure

This comment has been minimized.

Copy link
Member

commented Oct 20, 2018

@eslint/eslint-team Please take a look at the proposal here and (hopefully) give it a 👍. Thanks!

@nzakas nzakas added accepted and removed evaluating labels Nov 9, 2018

@nzakas

nzakas approved these changes Nov 9, 2018

@nzakas nzakas merged commit ae2b61d into eslint:master Nov 9, 2018

5 checks passed

commit-message PR title follows commit message guidelines
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
release-monitor No patch release is pending
Details
@nzakas

This comment has been minimized.

Copy link
Member

commented Nov 9, 2018

Thanks @pineapplemachine!

@eslint eslint bot locked and limited conversation to collaborators May 9, 2019

@eslint eslint bot added the archived due to age label May 9, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants
You can’t perform that action at this time.