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: enable linting for chained properties #7999

Closed
wants to merge 2 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Aug 6, 2016

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

tools

Description of change

Enforces linting for indentation of multiline chained properties. Requires updating to ESLint 3.2.2.

Refs: #7920

@Trott Trott added the tools Issues and PRs related to the tools directory. label Aug 6, 2016
@Trott
Copy link
Member Author

Trott commented Aug 6, 2016

(eslint was installed with --production and dmn -f clean was run on the results.)

@Trott
Copy link
Member Author

Trott commented Aug 6, 2016

/cc @silverwind

@Trott
Copy link
Member Author

Trott commented Aug 7, 2016

@Trott
Copy link
Member Author

Trott commented Aug 8, 2016

Raspberry Pi build-type failures in Ci. Certainly unrelated. But let's re-run anyway: https://ci.nodejs.org/job/node-test-pull-request/3560/

@cjihrig
Copy link
Contributor

cjihrig commented Aug 8, 2016

Rubber stamp LGTM on the ESLint update. Actual LGTM on the other commit.

@silverwind
Copy link
Contributor

LGTM

2 similar comments
@jasnell
Copy link
Member

jasnell commented Aug 8, 2016

LGTM

@jbergstroem
Copy link
Member

LGTM

@jasnell
Copy link
Member

jasnell commented Aug 9, 2016

More flaky failures in that last CI run but still unrelated. CI has been particularly flaky here this past week :-( /cc @nodejs/build

Will go ahead and land this but definitely need to keep an eye on things.

@jasnell
Copy link
Member

jasnell commented Aug 9, 2016

actually... hmmm.. @Trott, after applying this patch locally I'm getting:

$ make lint
Linting is not available through the source tarball.
Use the git repo instead: git clone https://github.com/nodejs/node.git

When I pull this patch back out it works.

@Trott
Copy link
Member Author

Trott commented Aug 9, 2016

Oh, interesting. The Makefile looks for the bin/eslint.js even though that file is not actually used in linting. I'll fix it.

@Trott
Copy link
Member Author

Trott commented Aug 9, 2016

OK, fixed make lint.

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

Trott added a commit to Trott/io.js that referenced this pull request Aug 10, 2016
PR-URL: nodejs#7999
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
Trott added a commit to Trott/io.js that referenced this pull request Aug 10, 2016
Refs: nodejs#7920
PR-URL: nodejs#7999
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
@Trott
Copy link
Member Author

Trott commented Aug 10, 2016

Landed in e313c02 and 8726a1c

@Trott Trott closed this Aug 10, 2016
@cjihrig cjihrig mentioned this pull request Aug 10, 2016
cjihrig pushed a commit that referenced this pull request Aug 10, 2016
PR-URL: #7999
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
cjihrig pushed a commit that referenced this pull request Aug 10, 2016
Refs: #7920
PR-URL: #7999
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
@cjihrig cjihrig mentioned this pull request Aug 11, 2016
@Trott Trott deleted the eslint3.2.2 branch October 10, 2021 01:39
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.

6 participants