Update: space-in-parens linting messages and report behaviour #11121
Conversation
|
|
eslint-deprecated
bot
added
the
triage
GrayedFox
force-pushed
the
issue10905
branch
from
555b4df
to
65d126c
Compare
platinumazure
added
bug
|
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! |
|
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 |
|
@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! |
|
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 😉 |
|
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! |
|
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! |
|
Sounds good @GrayedFox, ping me when you're ready for me to take another look! |
GrayedFox
changed the title
GrayedFox
changed the title
|
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 |
GrayedFox
changed the title
GrayedFox
changed the title
platinumazure
added
enhancement
There was a problem hiding this comment.
LGTM, just left two minor notes.
Unrelated to this PR, It's strange that only always without the empty exception doesn't enforce anything on empty parens, but it's already working like that.
always + empty exception requires ()
never also requires ()
never + empty exception requires ( )
Only always allows both () and ( )
To avoid confusion, nothing should be changed in this PR regarding this 'inconsistency'
lib/rules/space-in-parens.js
Outdated
| * @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 |
There was a problem hiding this comment.
This applies to all 4 similar functions, the description should be a bit more specific, e.g. 'has a disallowed space'.
There was a problem hiding this comment.
Nice, good catch, forgot to update the description there 👍
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ah, I thought the problem was with the description of the boolean return variable, which I updated - have updated all descriptions to be consistent, let me know what you think
|
Is there another issue for the breaking change to resolve the inconsistency? |
lib/rules/space-in-parens.js
Outdated
| * @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 |
There was a problem hiding this comment.
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.
GrayedFox
force-pushed
the
issue10905
branch
2 times, most recently
from
a15fe4e
to
f64566f
Compare
This also updates the rule description to be clearer, and refactors the code to use messageIds instead of constants. Removes some superfluous code.
GrayedFox
force-pushed
the
issue10905
branch
from
f64566f
to
db68e64
Compare
I agree that this is a non-breaking change. It's +1 error when there is already an error. |
There is an open issue #12141, for several rules with similar behavior. |
|
Merged! Thanks @GrayedFox for sticking with this and being so responsive to feedback! |
|
@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 💃 |
eslint-deprecated
bot
added
the
archived due to age
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 ifblocks. 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 settingsfoo( )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.