New Rule: requireSpacesInsideParenthesizedExpression #1254

gibson042 · 2015-04-10T09:03:48Z

A proof of concept for distinguishing the several parenthetical productions (statement affix, function definition parameters, function call arguments, and arbitrary expressions), inspired by https://github.com/jquery/sizzle/pull/330/files#r27985694 . This PR implements only requireSpacesInsideParenthesizedExpression, but if you like the concepts herein I will add disallowSpacesInsideParenthesizedExpression so it can merge.

The obvious next step is to introduce similar pairs for SpacesInsideParenthesizedArguments, SpacesInsideParenthesizedParameters, and SpacesInsideParenthesizedStatement (names subject to change, but chosen for convenient alphabetic grouping) in a pattern analogous to that embodied by the "SpacesInFunction" rules but rooted at "SpacesInsideParentheses".

I believe this also relates to existing work:
Ref #1069 (comment)
Ref gh-812
Ref gh-431

coveralls · 2015-04-10T09:06:14Z

Coverage decreased (-1.12%) to 97.97% when pulling 38bbab1 on gibson042:spaces-inside-parenthesized-expression into da3cb06 on jscs-dev:master.

coveralls · 2015-04-12T18:38:19Z

Coverage decreased (-1.07%) to 98.03% when pulling e04a4a9 on gibson042:spaces-inside-parenthesized-expression into da3cb06 on jscs-dev:master.

gibson042 · 2015-04-21T02:54:17Z

@jscs-dev is there something more to do for feedback on this?

- This effectively implements our "Embrace HTML5" option - Related: http://goo.gl/GcQAtn Fixes gh-2257

mrjoelkemp · 2015-06-21T15:13:56Z

@gibson042 Sorry this slipped through the cracks. This needs to be rebased and the coverage needs to be increased.

@hzoo Does this look good aside from the above issues? I see it's holding up the Idiomatic Preset PR: #1069. Would like to land both (this and Idiomatic) prs.

gibson042 · 2015-07-01T14:47:09Z

Rebase complete. Why did the coverage report not pick up use of the new util functions by rules requiring them, though? But at any rate, they are now specifically tested.

mikesherov · 2015-07-01T20:19:24Z

Why did the coverage report not pick up use of the new util functions by rules requiring them, though?

That's a feature, not a bug. The unit coverage runner, written by @mdevils, explains the philosophy here: https://github.com/unit-coverage/unit-coverage#the-idea

gibson042 · 2015-07-24T19:37:43Z

Where does this PR stand? The missing coverage seemed reasonable to me (and consistent with preexisting files), but I can extend it if there's a non-decrease requirement. More than that, though, I want feedback on the approach and the ideas... as noted, this is a POC.

mrjoelkemp · 2015-07-25T02:37:32Z

lib/rules/require-spaces-inside-parenthesized-expression.js

+            var value = nextToken.value;
+
+            // Skip empty parentheses and explicit exceptions
+            if (value === ')' || value in exceptions) {


Why not set ')' as a default part of the exceptions object? You can then simplify this if statement.

Because it's only an exception in one direction—if I universally except ")", then the immediately following close-parenthesis logic would erroneously allow "((…))".

mrjoelkemp · 2015-07-25T02:49:06Z

This is really great work, @gibson042. Bravo.

gibson042 · 2015-07-25T16:40:43Z

Thanks for the review. Am I correct in assuming that this has legs and that I should round it out with the above TokenCategorizer suggestion and disallowSpacesInsideParenthesizedExpression inverse rule?

gibson042 · 2015-07-25T16:46:35Z

(and then the SpacesInsideParenthesizedArguments [function call], /SpacesInsideParenthesizedParameters [function definition], and SpacesInsideParenthesizedStatement [if/for/while/etc.] pairs, of course). Any thoughts on the names?

mrjoelkemp · 2015-07-25T17:37:58Z

Sounds like a plan to me. Maybe wait for the second go ahead from
@markelog, @hzoo, or @zxqfox.
On Jul 25, 2015 12:46 PM, "Richard Gibson" notifications@github.com wrote:

(and then the SpacesInsideParenthesizedArguments [function call], /
SpacesInsideParenthesizedParameters [function definition], and
SpacesInsideParenthesizedStatement [if/for/while/etc.] pairs, of
course). Any thoughts on the names?

—
Reply to this email directly or view it on GitHub
#1254 (comment).

qfox · 2015-07-25T20:39:33Z

test/specs/utils.js

+            '(((function(){ function f(){} if(0) return((f)(0, (1), ((2)))+0); throw(f()+(0)); })))'
+        );
+        var closeParens = [];
+        sharedFile.iterateTokenByValue(')', function(token) {


I believe it should be wrapped to before statement inside this describe.

Ref jscs-dev#1069 (comment) Ref jscs-devgh-812 Ref jscs-devgh-431

gibson042 · 2015-08-20T15:44:55Z

Ok, I believe this is ready. Points for special consideration:

comments handling: is configuration like allExcept: ['//', '/*', '*/'] reasonable?
naming: ParenthesizedExpression, ParenthesizedArguments, ParenthesizedParameters, and ParenthesizedStatement alphabetize conveniently, but may not be as intuitive as e.g. ParenthesizedExpression, Call(Expression)?Arguments, (Function)?Parameters, and StatementParentheses

markelog · 2015-08-20T16:13:50Z

How would we enforce

(options || { x: true } ).x

but disallow

(options || { x: true }).x

?

markelog · 2015-08-20T16:22:13Z

test/specs/rules/require-spaces-inside-parenthesized-expression.js

+            assert(checker.checkString('(//comment\n foo //comment\n)').getErrorCount() === 1);
+        });
+
+        it('should not look inside comments', function() {


is this description correct?

Yes. These rules should look only at the characters beginning or ending comments, not at their contents.

markelog · 2015-08-20T16:27:29Z

Did you run it on core source?

comments handling: is configuration like allExcept: ['//', '/', '/'] reasonable?

It is for me, don't see any better way

Naming: ParenthesizedExpression, ParenthesizedArguments, ParenthesizedParameters, and ParenthesizedStatement alphabetize conveniently, but may not be as intuitive as e.g. ParenthesizedExpression, Call(Expression)?Arguments, (Function)?Parameters, and StatementParentheses

What would those rules do? Could you give me couple examples?

gibson042 · 2015-08-20T18:20:21Z

How would we enforce

(options || { x: true } ).x

but disallow

(options || { x: true }).x

?

I'm not sure I understand... can you provide an English explanation of desired behavior? I've followed the other JSCS patterns, so it's possible to require spaces with a finite set of exceptions and also possible to disallow spaces with a finite set of exceptions, but AFAIK not possible to require spaces only for a finite set of "inside" characters.

Did you run it on core source?

For jQuery? Not yet.

Naming: ParenthesizedExpression, ParenthesizedArguments, ParenthesizedParameters, and ParenthesizedStatement alphabetize conveniently, but may not be as intuitive as e.g. ParenthesizedExpression, Call(Expression)?Arguments, (Function)?Parameters, and StatementParentheses

What would those rules do? Could you give me couple examples?

ParenthesizedArguments or CallArguments or CallExpressionArguments: foo(‸a, b, …‸), foo.bar(‸a, b, …‸), (foo)(‸a, b, …‸), etc.
ParenthesizedParameters or FunctionParameters or Parameters: function(‸a, b, …‸) { … }, function foo(‸a, b, …‸) { … }, etc.
ParenthesizedStatement or StatementParentheses: if (‸…‸), for(‸…; …; …‸), catch (‸x‸), etc.
ParenthesizedExpression: everything else

gibson042 · 2015-08-21T04:55:06Z

I'm not sure I understand... can you provide an English explanation of desired behavior? I've followed the other JSCS patterns, so it's possible to require spaces with a finite set of exceptions and also possible to disallow spaces with a finite set of exceptions, but AFAIK not possible to require spaces only for a finite set of "inside" characters.

I stand corrected... disallowSpacesInsideParentheses has an only option, although it appears to be unique. Would it be acceptable to add and/or reassess that outside of this PR?

Did you run it on core source?

For jQuery? Not yet.

I checked master-branch jquery.js with { disallowSpacesInsideParenthesizedExpression: true }, and found over 100 exceptions (which is not surprising, because jQuery will likely use the other three rules and leave this one unspecified... cf. https://github.com/jquery/sizzle/pull/330/files#r27985694 ). However, file analysis took two minutes. I profiled jscs, and traced it down to inefficiencies in estraverse.traverse by way of JsFile#iterate by way of JsFile#getNodeByRange. Not wanting to dive into the dependency, I addressed the outermost of these in e97c8eb, which is technically out of scope for this PR but lowered runtime to 1.5 seconds (a reduction of 99%). If you'd rather see a separate PR for that, I could accommodate.

gibson042 · 2015-08-21T04:57:27Z

lib/rules/disallow-spaces-inside-parenthesized-expression.js

+            errors.assert.noWhitespaceBetween({
+                token: token,
+                nextToken: nextToken,
+                message: 'Illegal space after opening grouping round bracket'


I suppose this is also worth calling out. "Round bracket" reads weird to me, but it seems to be the terminology used here.

We had a discussion about it somewhere, for standardizing error output and rule names, in the new approach it would be cool to follow common terminology - https://en.wikipedia.org/wiki/Bracket.

In other words, i think it would be okay to call it "parentheses".

I think it was #698 (comment).

() - parentheses, [] - square brackets/brackets, {} - braces, <> - angle brackets

#538

markelog · 2015-08-21T15:25:27Z

Would it be acceptable to add and/or reassess that outside of this PR?

Totally

I addressed the outermost of these in e97c8eb, which is technically out of scope for this PR but lowered runtime to 1.5 seconds (a reduction of 99%).

Wow! That would be great to see it in the new release, i guess another PR would be best.

If no one has any objections, i will land this a bit later today.

Awesomely done Richard!

gibson042 · 2015-08-21T18:01:58Z

"Round bracket" references replaced and getNodeByRange improvement moved to a new PR.

markelog · 2015-08-22T17:35:51Z

Hooray!

gibson042 mentioned this pull request Apr 10, 2015

Build: Update grunt-jscs jquery/sizzle#330

Closed

gibson042 changed the title ~~[POC] requireSpacesInsideParenthesizedExpression: new rule~~ [POC] New Rule: requireSpacesInsideParenthesizedExpression Apr 12, 2015

gibson042 referenced this pull request in jquery/jquery May 4, 2015

Data: always camelCase keys in .data()

0e79098

- This effectively implements our "Embrace HTML5" option - Related: http://goo.gl/GcQAtn Fixes gh-2257

gibson042 force-pushed the spaces-inside-parenthesized-expression branch from e04a4a9 to ace28ea Compare July 1, 2015 14:36

gibson042 mentioned this pull request Jul 13, 2015

Attributes: .removeClass() -> .attr( "class", "" ) jquery/jquery#2465

Closed

mrjoelkemp reviewed Jul 25, 2015
View reviewed changes

qfox reviewed Jul 25, 2015
View reviewed changes

markelog added the WIP label Aug 11, 2015

gibson042 mentioned this pull request Aug 16, 2015

Update code style, partially use new qunit interface, give test module a new name jquery/jquery#2542

Closed

gibson042 added 9 commits August 18, 2015 17:42

requireSpacesInsideParenthesizedExpression: new rule test cases

a1d5fb5

Ref jscs-dev#1069 (comment) Ref jscs-devgh-812 Ref jscs-devgh-431

requireSpacesInsideParenthesizedExpression: new rule implementation

ac31d6b

Ref jscs-dev#1069 (comment) Ref jscs-devgh-812 Ref jscs-devgh-431

Improve coverage

70d5687

Specifically test categorizeOpenParen

6eee0c6

Specifically test categorizeCloseParen

0adbae3

Fix test failures

d0c5f94

Name variables more accurately

a5a487b

Move setup logic into before

3f6e85f

Move parenthesis categorization into a new class

04af8e6

gibson042 added 2 commits August 20, 2015 10:36

Properly handle comments

c9291c2

opposite rule: disallowSpacesInsideParenthesizedExpression

02404f1

gibson042 force-pushed the spaces-inside-parenthesized-expression branch from ace28ea to 02404f1 Compare August 20, 2015 15:29

markelog reviewed Aug 20, 2015
View reviewed changes

Handle NewExpression <: CallExpression

d517283

gibson042 reviewed Aug 21, 2015
View reviewed changes

This was referenced Aug 21, 2015

JsFile#getNodeByRange doesn't use the prebuilt index #1712

Closed

JsFile: Improve getNodeByRange performance #1713

Closed

Use "parenthesis" instead of "round bracket"

5b98b26

gibson042 force-pushed the spaces-inside-parenthesized-expression branch from e97c8eb to 5b98b26 Compare August 21, 2015 17:24

gibson042 changed the title ~~[POC] New Rule: requireSpacesInsideParenthesizedExpression~~ New Rule: requireSpacesInsideParenthesizedExpression Aug 21, 2015

markelog removed the WIP label Aug 22, 2015

markelog closed this in 1acc0c0 Aug 22, 2015

This was referenced Nov 3, 2015

New rule: Spacing for function params #1954

Closed

Add rules for spaces inside function calls [WIP] #1951

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Rule: requireSpacesInsideParenthesizedExpression #1254

New Rule: requireSpacesInsideParenthesizedExpression #1254

gibson042 commented Apr 10, 2015

coveralls commented Apr 10, 2015

coveralls commented Apr 12, 2015

gibson042 commented Apr 21, 2015

mrjoelkemp commented Jun 21, 2015

gibson042 commented Jul 1, 2015

mikesherov commented Jul 1, 2015

gibson042 commented Jul 24, 2015

mrjoelkemp Jul 25, 2015

gibson042 Jul 25, 2015

mrjoelkemp commented Jul 25, 2015

gibson042 commented Jul 25, 2015

gibson042 commented Jul 25, 2015

mrjoelkemp commented Jul 25, 2015

qfox Jul 25, 2015

gibson042 commented Aug 20, 2015

markelog commented Aug 20, 2015

markelog Aug 20, 2015

gibson042 Aug 20, 2015

markelog commented Aug 20, 2015

gibson042 commented Aug 20, 2015

gibson042 commented Aug 21, 2015

gibson042 Aug 21, 2015

markelog Aug 21, 2015

hzoo Aug 21, 2015

markelog commented Aug 21, 2015

gibson042 commented Aug 21, 2015

markelog commented Aug 22, 2015

New Rule: requireSpacesInsideParenthesizedExpression #1254

New Rule: requireSpacesInsideParenthesizedExpression #1254

Conversation

gibson042 commented Apr 10, 2015

coveralls commented Apr 10, 2015

coveralls commented Apr 12, 2015

gibson042 commented Apr 21, 2015

mrjoelkemp commented Jun 21, 2015

gibson042 commented Jul 1, 2015

mikesherov commented Jul 1, 2015

gibson042 commented Jul 24, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrjoelkemp commented Jul 25, 2015

gibson042 commented Jul 25, 2015

gibson042 commented Jul 25, 2015

mrjoelkemp commented Jul 25, 2015

Choose a reason for hiding this comment

gibson042 commented Aug 20, 2015

markelog commented Aug 20, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markelog commented Aug 20, 2015

gibson042 commented Aug 20, 2015

gibson042 commented Aug 21, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markelog commented Aug 21, 2015

gibson042 commented Aug 21, 2015

markelog commented Aug 22, 2015