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

Eslint 4.1.0 #13895

Closed
wants to merge 4 commits into from
Closed

Eslint 4.1.0 #13895

wants to merge 4 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Jun 24, 2017

First commit:

tools: update to ESLint 4.1.0

Update ESLint to 4.1.0. This fixes a bug that previously prevented us
from using the new and stricter indentation checking.

Refs: https://github.com/eslint/eslint/issues/8721

Second commit:

tools: add script to update ESLint

Provide a bash script for updating ESLint in the project.

Third commit:

tools,benchmark: use stricter indentation linting

Enable stricter indentation rules for benchmark code.

Fourth commit:

tools: disable legacy indentation linting in tools

The tools directory has newer and stricter indentation enabled, but the
legacy indentation rules were not disabled. This could potentitally
result in a conflict between the two rule sets. Disable legacy linting.
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

tools benchmark

@Trott Trott added benchmark Issues and PRs related to the benchmark subsystem. tools Issues and PRs related to the tools directory. labels Jun 24, 2017

# This symlink will be pointed at the wrong place after all the moving after
# everything is moved around so let's remove it.
rm node_modules/.bin/eslint
Copy link
Member

Choose a reason for hiding this comment

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

Have we tried passing --no-bin-links to npm install to prevent the symlink being created in the first place?

Copy link
Member Author

Choose a reason for hiding this comment

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

Didn't know about that flag. Added! Thanks!

@@ -0,0 +1,30 @@
#!/bin/sh
Copy link
Member

Choose a reason for hiding this comment

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

Should this be bash instead of sh?

Copy link
Member

Choose a reason for hiding this comment

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

More specifically, #!/usr/bin/env bash?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that would be better, done, thanks!

@richardlau
Copy link
Member

👍 for scripting the update process.

@Trott Trott force-pushed the eslint-4.1.0 branch 2 times, most recently from ac3fbd7 to adb3ef9 Compare June 24, 2017 05:26
@Trott
Copy link
Member Author

Trott commented Jun 24, 2017

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

Not sure I love the node_modules shuffle trick

# eslint-plugin-markdown is pinned at 1.0.0-beta.4 until there is a release
# that fixes https://github.com/eslint/eslint-plugin-markdown/issues/69.
npm install --no-bin-links --production eslint@latest [email protected]
rm package-lock.json
Copy link
Contributor

Choose a reason for hiding this comment

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

Why? And if you really don't want it add --no-shrinkwrap to the line above.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, removed it the rm, thanks.

dmn -f clean

# Move stuff to where it goes in our source tree.
mv node_modules/eslint eslint
Copy link
Contributor

@refack refack Jun 24, 2017

Choose a reason for hiding this comment

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

A bit smelly?
Prefer to do in eslint-tmp.

Copy link
Member

Choose a reason for hiding this comment

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

I used --global-style to avoid having to move things in node_modules. The problem is I don't know how to install the markdown plugin this way.

@refack
Copy link
Contributor

refack commented Jun 24, 2017

Why not curl https://github.com/eslint/eslint/archive/v4.1.0.tar.gz then npm i ?
To me it seems more straightforward 🤷‍♂️

@Trott
Copy link
Member Author

Trott commented Jun 24, 2017

Why not curl https://github.com/eslint/eslint/archive/v4.1.0.tar.gz then npm i

@refack That assumes we know which version we want to update to. If there's an archive/latest.tar.gz or something like that, then that would work, I think.

@Trott
Copy link
Member Author

Trott commented Jun 24, 2017

(I don't want to have to update the script every time I wanted to update, but I guess if I had to, that's not exactly the end of the world. It would also document what version the script was last run with, in case someone starts updating without the script again. So maybe it's a good thing? I dunno...)

@Trott
Copy link
Member Author

Trott commented Jun 24, 2017

Another thing to consider: There's also the issue that since ESLint documents the npm install method only (at least as far as I can see), it might be a good idea (in theory at least?) to avoid devising our own method like that.

@refack
Copy link
Contributor

refack commented Jun 24, 2017

Another thing to consider: There's also the issue that since ESLint documents the npm install method only (at least as far as I can see), it might be a good idea (in theory at least?) to avoid devising our own method like that.

I'd assume they support npm i from git.... But I get what you're saying. Only thing I'd change then is to do the npm i eslint in a temp directory (e.g. eslint-temp) then move things out from there and rimraf it, maybe at some point we'll want to have tools/node_modules...

@Trott
Copy link
Member Author

Trott commented Jun 24, 2017

Good point about tools/node_modules...

Trott added 4 commits June 26, 2017 19:58
Update ESLint to 4.1.0. This fixes a bug that previously prevented us
from using the new and stricter indentation checking.

Refs: eslint/eslint#8721
Provide a bash script for updating ESLint in the project.
Enable stricter indentation rules for benchmark code.
The tools directory has newer and stricter indentation enabled, but the
legacy indentation rules were not disabled. This could potentitally
result in a conflict between the two rule sets. Disable legacy linting.
@Trott
Copy link
Member Author

Trott commented Jun 27, 2017

@targos
Copy link
Member

targos commented Jun 27, 2017

LGTM

@Trott
Copy link
Member Author

Trott commented Jun 27, 2017

Landed in aaee434 1e5e0ce 496f604
and b714060

@Trott Trott closed this Jun 27, 2017
@MylesBorins
Copy link
Contributor

@Trott backport?

@Trott
Copy link
Member Author

Trott commented Aug 14, 2017

Backport the first two but not the last two would be my suggestion. Coming in a few minutes...

Trott added a commit to Trott/io.js that referenced this pull request Aug 14, 2017
Update ESLint to 4.1.0. This fixes a bug that previously prevented us
from using the new and stricter indentation checking.

Refs: eslint/eslint#8721
PR-URL: nodejs#13895
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Trott added a commit to Trott/io.js that referenced this pull request Aug 14, 2017
Provide a bash script for updating ESLint in the project.

PR-URL: nodejs#13895
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@Trott Trott mentioned this pull request Aug 14, 2017
4 tasks
@Trott
Copy link
Member Author

Trott commented Aug 14, 2017

@MylesBorins #14830

Trott added a commit to Trott/io.js that referenced this pull request Aug 15, 2017
Update ESLint to 4.1.0. This fixes a bug that previously prevented us
from using the new and stricter indentation checking.

Refs: eslint/eslint#8721
PR-URL: nodejs#13895
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Trott added a commit to Trott/io.js that referenced this pull request Aug 15, 2017
Provide a bash script for updating ESLint in the project.

PR-URL: nodejs#13895
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
MylesBorins pushed a commit that referenced this pull request Aug 16, 2017
Update ESLint to 4.1.0. This fixes a bug that previously prevented us
from using the new and stricter indentation checking.

Refs: eslint/eslint#8721
PR-URL: #13895
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
MylesBorins pushed a commit that referenced this pull request Aug 16, 2017
Provide a bash script for updating ESLint in the project.

PR-URL: #13895
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
MylesBorins pushed a commit that referenced this pull request Aug 16, 2017
Update ESLint to 4.1.0. This fixes a bug that previously prevented us
from using the new and stricter indentation checking.

Refs: eslint/eslint#8721
Backport-PR-URL: #14830
PR-URL: #13895
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
MylesBorins pushed a commit that referenced this pull request Aug 16, 2017
Provide a bash script for updating ESLint in the project.

Backport-PR-URL: #14830
PR-URL: #13895
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Aug 16, 2017
MylesBorins pushed a commit that referenced this pull request Sep 5, 2017
Update ESLint to 4.1.0. This fixes a bug that previously prevented us
from using the new and stricter indentation checking.

Refs: eslint/eslint#8721
Backport-PR-URL: #14830
PR-URL: #13895
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
MylesBorins pushed a commit that referenced this pull request Sep 5, 2017
Provide a bash script for updating ESLint in the project.

Backport-PR-URL: #14830
PR-URL: #13895
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@Trott Trott deleted the eslint-4.1.0 branch January 13, 2022 22:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
benchmark Issues and PRs related to the benchmark subsystem. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants