tools: lint for object literal spacing #6592

Trott · 2016-05-05T05:26:37Z

Checklist

tests and code linting passes
the commit message follows commit guidelines

Affected core subsystem(s)

tools lib test

Description of change

There have been occasional nits for spacing in object literals in PRs but
the project does not lint for it and it is not always handled
consistently in the existing code, even on adjacent lines of a file.

This change enables a linting rule requiring no space between the key
and the colon, and requiring at least one space (but allowing for more
so property values can be lined up if desired) between the colon and the
value. This appears to be the most common style used in the current code
base.

Example code the complies with lint rule:

myObj = { foo: 'bar' };

Examples that do not comply with the lint rule:

myObj = { foo : 'bar' };
myObj = { foo:'bar' };

Trott · 2016-05-05T05:29:39Z

Sample nit for reference: #6005 (comment)

mscdex · 2016-05-05T05:35:54Z

lib/util.js

-  'magenta' : [35, 39],
-  'red' : [31, 39],
-  'yellow' : [33, 39]
+  'bold': [1, 22],


This is unrelated but what about having a lint rule to enforce non-quoted keys for keys that don't need quotes?

We could use http://eslint.org/docs/rules/quote-props for that.

mscdex · 2016-05-05T05:37:10Z

Did you use the -F linter argument to fix these?

Fishrock123 · 2016-05-05T14:15:05Z

Hmmm, I'm not really sure these are good enough gains when we still ignore all the other parts of object literals because there's just too much variance in the codebase.

Trott · 2016-05-05T16:06:58Z

Hmmm, I'm not really sure these are good enough gains when we still ignore all the other parts of object literals because there's just too much variance in the codebase.

This comment is vague and therefore difficult to understand or address.

Trott · 2016-05-05T16:10:38Z

Did you use the -F linter argument to fix these?

No. Surprisingly, key-spacing is not one of the rules that supports --fix. I'm not sure whether it's something easy that no one has bothered to do yet, or if it's a design decision because there's a lot of options for the rule and maybe some of them may be more difficult to auto-fix than others.

Fishrock123 · 2016-05-05T16:13:25Z

This comment is vague and therefore difficult to understand or address.

It's confusing we only address space-before colon when we don't do anything about space after colon, or space between braces, or possibly even space after comma(?).

All in all this is an awful lot of churn that is only partial, since it doesn't even address the others (which, iirc are too large to address).

As such, I'd vote we do nothing about it, probably, while trying to move over in a slower fashion.

mscdex · 2016-05-05T16:15:13Z

test/parallel/test-tls-alert.js

@@ -28,8 +28,8 @@ function loadPEM(n) {

 var server = tls.Server({
  secureProtocol: 'TLSv1_2_server_method',
-  key: loadPEM('agent2-key'),
-  cert:loadPEM('agent2-cert')
+  key:  loadPEM('agent2-key'),


Why did this need to change?

It didn't need to change, but it appeared to me that the inconsistent spacing of the original was an attempt to align the values in a single column, so I did that. Don't really care either way TBH.

mscdex · 2016-05-05T16:18:26Z

A couple of nits but otherwise LGTM

mscdex · 2016-05-05T16:27:47Z

As such, I'd vote we do nothing about it, probably, while trying to move over in a slower fashion.

@Fishrock123 Wouldn't incorporating those other changes later on be accomplishing things in a "slower fashion?" It seems like adding all of the other related rules at the same time would be contrary to this.

Trott · 2016-05-05T16:48:42Z

@Fishrock123 Thanks for clarifying. We lint for all the things you describe except one.

It's confusing we only address space-before colon when we don't do anything about space after colon

We do. A space after the colon is required with the rule in this PR.

Good: {foo: bar}

Bad: {foo:bar}

or space between braces

I don't think it would be confusing if this PR were to land resulting in us linting for spacing in the key/value pairs but not for the curly braces. Admittedly, that's a subjective assessment, though.

Anyway, that would be object-curly-spacing. If we wanted to enforce that, it would affect around 200 files (depending on configuration options). On the upside, the -F option will work there, so if we were OK with that much churn for a spacing rule (which: no), it could happen really fast...

possibly even space after comma(?).

We lint for comma spacing.

Bad: {foo: bar,baz: bip}

Good: {foo: bar, baz: bip}

As such, I'd vote we do nothing about it, probably, while trying to move over in a slower fashion.

Thanks again for explaining.

I'll throw in one last argument in favor of the change: I cannot help but cringe every time a new contributor gets nits for things that really ought to be caught by tooling. As a project, we've gotten a lot better about not giving every new committer 150 style nits. Honestly, I'm not sure that's because we're any better behaved or if it's because we just have more lint rules now that catch stuff or what. Regardless, I would happily accept a little churn (and this is only 35 files, almost all tests, and most of which only have one or two lines of change) if it means less churn in PRs on stuff that shouldn't matter.

jasnell · 2016-05-06T17:10:49Z

Yes please. Definitely happy with this one. LGTM

Fishrock123 · 2016-05-06T17:49:58Z

Seems fine to me.

There has been occasional nits for spacing in object literals in PRs but the project does not lint for it and it is not always handled consistently in the existing code, even on adjacent lines of a file. This change enables a linting rule requiring no space between the key and the colon, and requiring at least one space (but allowing for more so property values can be lined up if desired) between the colon and the value. This appears to be the most common style used in the current code base. Example code the complies with lint rule: myObj = { foo: 'bar' }; Examples that do not comply with the lint rule: myObj = { foo : 'bar' }; myObj = { foo:'bar' };

Trott · 2016-05-09T02:30:10Z

Rebased against master, addressed the nits, squashed, and force pushed.

CI: https://ci.nodejs.org/job/node-test-pull-request/2540/

mscdex · 2016-05-09T03:59:58Z

Still LGTM

There has been occasional nits for spacing in object literals in PRs but the project does not lint for it and it is not always handled consistently in the existing code, even on adjacent lines of a file. This change enables a linting rule requiring no space between the key and the colon, and requiring at least one space (but allowing for more so property values can be lined up if desired) between the colon and the value. This appears to be the most common style used in the current code base. Example code the complies with lint rule: myObj = { foo: 'bar' }; Examples that do not comply with the lint rule: myObj = { foo : 'bar' }; myObj = { foo:'bar' }; PR-URL: nodejs#6592 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>

Trott · 2016-05-09T05:46:57Z

Landed in 4e6dc00

There has been occasional nits for spacing in object literals in PRs but the project does not lint for it and it is not always handled consistently in the existing code, even on adjacent lines of a file. This change enables a linting rule requiring no space between the key and the colon, and requiring at least one space (but allowing for more so property values can be lined up if desired) between the colon and the value. This appears to be the most common style used in the current code base. Example code the complies with lint rule: myObj = { foo: 'bar' }; Examples that do not comply with the lint rule: myObj = { foo : 'bar' }; myObj = { foo:'bar' }; PR-URL: #6592 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>

There has been occasional nits for spacing in object literals in PRs but the project does not lint for it and it is not always handled consistently in the existing code, even on adjacent lines of a file. This change enables a linting rule requiring no space between the key and the colon, and requiring at least one space (but allowing for more so property values can be lined up if desired) between the colon and the value. This appears to be the most common style used in the current code base. Example code the complies with lint rule: myObj = { foo: 'bar' }; Examples that do not comply with the lint rule: myObj = { foo : 'bar' }; myObj = { foo:'bar' }; PR-URL: nodejs#6592 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>

There has been occasional nits for spacing in object literals in PRs but the project does not lint for it and it is not always handled consistently in the existing code, even on adjacent lines of a file. This change enables a linting rule requiring no space between the key and the colon, and requiring at least one space (but allowing for more so property values can be lined up if desired) between the colon and the value. This appears to be the most common style used in the current code base. Example code the complies with lint rule: myObj = { foo: 'bar' }; Examples that do not comply with the lint rule: myObj = { foo : 'bar' }; myObj = { foo:'bar' }; PR-URL: #6592 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>

Trott added test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. lib / src Issues and PRs related to general changes in the lib or src directory. labels May 5, 2016

mscdex reviewed May 5, 2016
View reviewed changes

Trott force-pushed the keyspace branch from 18635b8 to b74d091 Compare May 9, 2016 02:25

Trott closed this May 9, 2016

Trott mentioned this pull request May 9, 2016

tools: disallow multiple spaces except for indentation #6645

Closed

2 tasks

evanlucas mentioned this pull request May 17, 2016

2016-05-17, Version 6.2.0 (Stable) #6810

Merged

MylesBorins added the dont-land-on-v4.x label Jun 2, 2016

addaleax mentioned this pull request Jul 12, 2016

Bring the doctool up to date on v4.x #7668

Merged

MylesBorins mentioned this pull request Jul 12, 2016

v4.5.0 proposal #7688

Merged

Trott deleted the keyspace branch January 13, 2022 22:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tools: lint for object literal spacing #6592

tools: lint for object literal spacing #6592

Trott commented May 5, 2016 •

edited

Loading

Trott commented May 5, 2016

mscdex May 5, 2016

cjihrig May 5, 2016

mscdex commented May 5, 2016 •

edited

Loading

Fishrock123 commented May 5, 2016

Trott commented May 5, 2016

Trott commented May 5, 2016

Fishrock123 commented May 5, 2016

mscdex May 5, 2016

Trott May 5, 2016

mscdex commented May 5, 2016

mscdex commented May 5, 2016

Trott commented May 5, 2016

jasnell commented May 6, 2016

Fishrock123 commented May 6, 2016

Trott commented May 9, 2016

mscdex commented May 9, 2016

Trott commented May 9, 2016

tools: lint for object literal spacing #6592

tools: lint for object literal spacing #6592

Conversation

Trott commented May 5, 2016 • edited Loading

Checklist

Affected core subsystem(s)

Description of change

Trott commented May 5, 2016

mscdex May 5, 2016

Choose a reason for hiding this comment

cjihrig May 5, 2016

Choose a reason for hiding this comment

mscdex commented May 5, 2016 • edited Loading

Fishrock123 commented May 5, 2016

Trott commented May 5, 2016

Trott commented May 5, 2016

Fishrock123 commented May 5, 2016

mscdex May 5, 2016

Choose a reason for hiding this comment

Trott May 5, 2016

Choose a reason for hiding this comment

mscdex commented May 5, 2016

mscdex commented May 5, 2016

Trott commented May 5, 2016

jasnell commented May 6, 2016

Fishrock123 commented May 6, 2016

Trott commented May 9, 2016

mscdex commented May 9, 2016

Trott commented May 9, 2016

Trott commented May 5, 2016 •

edited

Loading

mscdex commented May 5, 2016 •

edited

Loading