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

tools: lint for object literal spacing #6592

Closed
wants to merge 1 commit into from
Closed

Conversation

Trott
Copy link
Member

@Trott Trott commented May 5, 2016

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 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
@Trott
Copy link
Member Author

Trott commented May 5, 2016

Sample nit for reference: #6005 (comment)

'magenta' : [35, 39],
'red' : [31, 39],
'yellow' : [33, 39]
'bold': [1, 22],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mscdex
Copy link
Contributor

mscdex commented May 5, 2016

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

@Fishrock123
Copy link
Contributor

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
Copy link
Member Author

Trott commented May 5, 2016

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
Copy link
Member Author

Trott commented May 5, 2016

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
Copy link
Contributor

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.

@@ -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'),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did this need to change?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

mscdex commented May 5, 2016

A couple of nits but otherwise LGTM

@mscdex
Copy link
Contributor

mscdex commented May 5, 2016

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
Copy link
Member Author

Trott commented May 5, 2016

@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
Copy link
Member

jasnell commented May 6, 2016

Yes please. Definitely happy with this one. LGTM

@Fishrock123
Copy link
Contributor

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
Copy link
Member Author

Trott commented May 9, 2016

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

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

@mscdex
Copy link
Contributor

mscdex commented May 9, 2016

Still LGTM

Trott added a commit to Trott/io.js that referenced this pull request May 9, 2016
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
Copy link
Member Author

Trott commented May 9, 2016

Landed in 4e6dc00

@Trott Trott closed this May 9, 2016
evanlucas pushed a commit that referenced this pull request May 17, 2016
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>
addaleax pushed a commit to addaleax/node that referenced this pull request Jul 12, 2016
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>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
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>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
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>
@MylesBorins MylesBorins mentioned this pull request Jul 12, 2016
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
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>
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
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 Trott deleted the keyspace branch January 13, 2022 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants