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: make argument alignment linting more strict #8642

Merged
merged 2 commits into from
Sep 20, 2016

Conversation

Trott
Copy link
Member

@Trott Trott commented Sep 18, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

tools test lib

Description of change

A few nits in recent PR comments suggest that we can have slightly more
strict linting for argument alignment in multiline function calls. This
enables the existing linting requirements to apply when one or more of
the arguments themselves are function calls. Previously, that situation
had been excluded from linting.

@Trott Trott added the tools Issues and PRs related to the tools directory. label Sep 18, 2016
@nodejs-github-bot nodejs-github-bot added repl Issues and PRs related to the REPL subsystem. tools Issues and PRs related to the tools directory. labels Sep 18, 2016
@Trott
Copy link
Member Author

Trott commented Sep 18, 2016

@Fishrock123
Copy link
Contributor

Hmmm, I remember this being a lot worse when we tried it before? (I think we tried it before?)

Copy link
Contributor

@not-an-aardvark not-an-aardvark left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

LGTM, and thank you!

@addaleax addaleax removed the repl Issues and PRs related to the REPL subsystem. label Sep 18, 2016
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@imyller imyller left a comment

Choose a reason for hiding this comment

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

LGTM

@fhinkel
Copy link
Member

fhinkel commented Sep 20, 2016

LGTM.

An upcoming custom lint rule will provide slightly more strict
enforcement of argument alignment for multiline function calls. Adjust
existing code to conform.

PR-URL: nodejs#8642
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
A few nits in recent PR comments suggest that we can have slightly more
strict linting for argument alignment in multiline function calls. This
enables the existing linting requirements to apply when one or more of
the arguments themselves are function calls. Previously, that situation
had been excluded from linting.

Refs: nodejs#8628 (comment)
PR-URL: nodejs#8642
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
@Trott Trott merged commit f785b36 into nodejs:master Sep 20, 2016
@Trott
Copy link
Member Author

Trott commented Sep 20, 2016

Landed in 4316f4d and f785b36

@MylesBorins
Copy link
Contributor

@Trott I'm going to stop landing these changes to the linter in v4.x

Please feel free to make a mass PR to get the two line in sync

@Trott
Copy link
Member Author

Trott commented Oct 6, 2016

@thealphanerd I'm OK with the lint rules in v4.x being more lax than in master, so that's OK with me. If you'd prefer it land, I can try to backport. But if it's not high value to you, then I'm content to leave it out.

@MylesBorins
Copy link
Contributor

I say leave it

Fishrock123 pushed a commit that referenced this pull request Oct 11, 2016
An upcoming custom lint rule will provide slightly more strict
enforcement of argument alignment for multiline function calls. Adjust
existing code to conform.

PR-URL: #8642
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Fishrock123 pushed a commit that referenced this pull request Oct 11, 2016
A few nits in recent PR comments suggest that we can have slightly more
strict linting for argument alignment in multiline function calls. This
enables the existing linting requirements to apply when one or more of
the arguments themselves are function calls. Previously, that situation
had been excluded from linting.

Refs: #8628 (comment)
PR-URL: #8642
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
@Trott Trott deleted the align-moar branch January 13, 2022 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants