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: space-in-parens linting messages and report behaviour #11121

Merged
merged 1 commit into from Sep 6, 2019

Conversation

@GrayedFox
Copy link
Contributor

GrayedFox commented Nov 24, 2018

What is the purpose of this pull request? (put an "X" next to item)

The current space-in-parens description is a little unclear. Technically empty parens can trigger an error if specified in the exceptions options. This PR updates that line in the docs and adds a paragraph that attempts to explain what exactly this rule does. Current rule description.

The linting error messages are now more specific and inform whether the paren token is either rejecting an extra space or missing a required space. Previously the output did not specify if the space was required or missing (so it could be either).

The previous context/report behavior would sometimes report only the first error encountered within a node due to the token checks being nested inside else if blocks. This is a non-breaking change: it simply means all errors will be reported within a node instead of only one - it does not introduce a new error type. The fallout of this change is that there is now at least one interesting case: using the default settings foo( ) technically contains two errors: that single space is both a rejected opening and closing space, so two errors will be reported despite the fact that removing the one space fixes both errors.

@jsf-clabot

This comment has been minimized.

Copy link

jsf-clabot commented Nov 24, 2018

CLA assistant check
All committers have signed the CLA.

@eslint eslint bot added the triage label Nov 24, 2018
@GrayedFox GrayedFox force-pushed the GrayedFox:issue10905 branch from 555b4df to 65d126c Nov 24, 2018
@platinumazure

This comment has been minimized.

Copy link
Member

platinumazure commented Dec 7, 2018

Hi @GrayedFox, thanks for the PR! Apologies for letting this slip through the cracks.

I've gone through the comments you left in the issue after my latest comment there and now I have a decent idea what's going on here. I'll try to review this tonight and will let you know if I have any questions here. Thanks so much for your patience!

@platinumazure platinumazure self-assigned this Dec 7, 2018
@GrayedFox

This comment has been minimized.

Copy link
Contributor Author

GrayedFox commented Dec 10, 2018

Hey there, no worries! I've started doing a relatively significant refactor of this rule, since I found the code incredibly hard to read and follow (although it's making more sense now). The last push I did breaks about 50 tests so it's still very much a WIP! Will likely have time to do a bit more work on it tonight/this week

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Jan 16, 2019

@GrayedFox just checking in to see if you still intend to finish this up? No worries if not, we'd just like to figure out what to do with this PR. Thanks!

@GrayedFox

This comment has been minimized.

Copy link
Contributor Author

GrayedFox commented Jan 16, 2019

Hi there! I do, and thanks for the patience - had a nice holiday period and am still wanting to complete this, very much.

Can do a little more work tonight actually just to get the ball rolling again. At the bare minimum, this PR would act as a clean up to the current space-in-parens logic and output more helpful messages, but I'm still aiming to fix what I think is buggy behaviour.

Thanks for checking in 😉

@GrayedFox

This comment has been minimized.

Copy link
Contributor Author

GrayedFox commented Feb 6, 2019

Cleaned up the PR description and removed my old out of date comments - it's taken me a long time to properly wrap my head around what I think the bugged behaviour is - and even longer to really understand the internals of the space-in-parens file and tests (it's a jungle out there!).

This PR now has a clear goal in mind and outlines intended changes. Thanks for your patience!

@aladdin-add aladdin-add requested a review from platinumazure May 26, 2019
Copy link
Member

platinumazure left a comment

LGTM, thanks! And I'm very sorry for losing track of this!

@platinumazure

This comment has been minimized.

Copy link
Member

platinumazure commented May 26, 2019

Oh, but this will probably need to be rebased and the commit message fixed.

Commit message should start with "Update:" and should explain what is fixed as briefly as possible. You can add more info later in the commit message (outside the first line) if needed. Thanks!

@GrayedFox GrayedFox force-pushed the GrayedFox:issue10905 branch from 41e8a8b to 7fd8713 May 26, 2019
@platinumazure

This comment has been minimized.

Copy link
Member

platinumazure commented May 26, 2019

Sounds good @GrayedFox, ping me when you're ready for me to take another look!

@GrayedFox GrayedFox changed the title (WIP) fix space-in-parens Fix space-in-parens to really enforce consistent spacing Aug 10, 2019
@GrayedFox GrayedFox force-pushed the GrayedFox:issue10905 branch from dcca713 to 213b179 Aug 10, 2019
@GrayedFox GrayedFox changed the title Fix space-in-parens to really enforce consistent spacing Fix: space-in-parens to really enforce consistent spacing (fixes #10905) Aug 10, 2019
@GrayedFox

This comment has been minimized.

Copy link
Contributor Author

GrayedFox commented Aug 10, 2019

Finally got around to finishing this. Updated docs to follow suit (this does have breaking changes in the sense that previously green code could now go red after updating to this version). This was a necessary result of getting the rule to properly enforce consistent spacing, even with outer most parentheses, but it comes with another advantage: the "empty" exception now functions exactly as described (by actually allowing or disallowing empty parens).

Previously the core rule description was also a bit misleading, since it stated that () would always be allowed, but this is not quite true, since users could specify //"never", { "exceptions": ["empty"] }] and that would disallow empty parentheses (funnily enough this exact sample is documented).

@GrayedFox GrayedFox changed the title Fix: space-in-parens to really enforce consistent spacing (fixes #10905) WIP: space-in-parens to really enforce consistent spacing (fixes #10905) (updated docs coming soon) Aug 10, 2019
@GrayedFox GrayedFox force-pushed the GrayedFox:issue10905 branch from b26383c to 91b470b Aug 12, 2019
@GrayedFox GrayedFox changed the title WIP: space-in-parens to really enforce consistent spacing (fixes #10905) (updated docs coming soon) Fix: space-in-parens to enforce consistent spacing within outer parens (fixes #10905) Aug 12, 2019
@GrayedFox GrayedFox force-pushed the GrayedFox:issue10905 branch from 91b470b to 1e92180 Aug 12, 2019
@GrayedFox GrayedFox closed this Aug 12, 2019
@GrayedFox GrayedFox force-pushed the GrayedFox:issue10905 branch from 1e92180 to a675c89 Aug 12, 2019
@GrayedFox GrayedFox reopened this Aug 12, 2019
@GrayedFox GrayedFox force-pushed the GrayedFox:issue10905 branch from 3cf80c7 to b754d54 Aug 30, 2019
@platinumazure

This comment has been minimized.

Copy link
Member

platinumazure commented Aug 30, 2019

Is there another issue for the breaking change to resolve the inconsistency?

Copy link
Member

mdjermanovic left a comment

Few notes regarding the comments and the documentation.

* @param {Object} left The token before the paren
* @param {Object} right The paren token
* @returns {boolean} True if the paren should have a space
* Determines if an opening paren has a space immediately after it

This comment has been minimized.

Copy link
@mdjermanovic

mdjermanovic Aug 30, 2019

Member

I think it's still the same description? It should be more specific, like:

Determines if an opening paren has a disallowed space immediately after it

because it doesn't determine only if an opening paren has a space immediately after it.

lib/rules/space-in-parens.js Outdated Show resolved Hide resolved
lib/rules/space-in-parens.js Outdated Show resolved Hide resolved
lib/rules/space-in-parens.js Outdated Show resolved Hide resolved
lib/rules/space-in-parens.js Outdated Show resolved Hide resolved
docs/rules/space-in-parens.md Outdated Show resolved Hide resolved
@GrayedFox GrayedFox force-pushed the GrayedFox:issue10905 branch 2 times, most recently from a15fe4e to f64566f Sep 1, 2019
This also updates the rule description to be clearer, and refactors the 
code to use messageIds instead of constants. Removes some superfluous 
code.
@GrayedFox GrayedFox force-pushed the GrayedFox:issue10905 branch from f64566f to db68e64 Sep 1, 2019
@mdjermanovic

This comment has been minimized.

Copy link
Member

mdjermanovic commented Sep 1, 2019

The previous context/report behavior would sometimes report only the first error encountered within a node due to the token checks being nested inside else if blocks. This is a non-breaking change: it simply means all errors will be reported within a node instead of only one - it does not introduce a new error type. The fallout of this change is that there is now at least one interesting case: using the default settings foo( ) technically contains two errors: that single space is both a rejected opening and closing space, so two errors will be reported despite the fact that removing the one space fixes both errors.

I agree that this is a non-breaking change. It's +1 error when there is already an error.

@mdjermanovic

This comment has been minimized.

Copy link
Member

mdjermanovic commented Sep 1, 2019

Is there another issue for the breaking change to resolve the inconsistency?

There is an open issue #12141, for several rules with similar behavior.

Copy link
Member

mdjermanovic left a comment

LGTM, thanks!

@platinumazure platinumazure merged commit 7621f5d into eslint:master Sep 6, 2019
9 checks passed
9 checks passed
commit-message Commit message follows guidelines
Details
continuous-integration Build #20190901.6 succeeded
Details
continuous-integration (Test on Node.js 10 (Linux)) Test on Node.js 10 (Linux) succeeded
Details
continuous-integration (Test on Node.js 12 (Linux)) Test on Node.js 12 (Linux) succeeded
Details
continuous-integration (Test on Node.js 12 (Windows)) Test on Node.js 12 (Windows) succeeded
Details
continuous-integration (Test on Node.js 12 (macOS)) Test on Node.js 12 (macOS) succeeded
Details
continuous-integration (Test on Node.js 8 (Linux)) Test on Node.js 8 (Linux) succeeded
Details
licence/cla Contributor License Agreement is signed.
Details
release-monitor No patch release is pending
Details
@platinumazure

This comment has been minimized.

Copy link
Member

platinumazure commented Sep 6, 2019

Merged! Thanks @GrayedFox for sticking with this and being so responsive to feedback!

@GrayedFox

This comment has been minimized.

Copy link
Contributor Author

GrayedFox commented Sep 7, 2019

@platinumazure @mdjermanovic my pleasure! happy to both receive and respond to feedback, that's what keeps the ship running squeaky clean in the first place 💃

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.