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.

Sign up for GitHub

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

Jump to bottom

tools: enable final newline in .editorconfig #9410

Closed
wants to merge 2 commits into from
Closed

tools: enable final newline in .editorconfig #9410

wants to merge 2 commits into from

Conversation

silverwind
Copy link
Contributor

@silverwind silverwind commented Nov 1, 2016 •
edited
Loading

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

tools

Description of change

This enables the final newline insertion for editors supporting .editorconfig, which hopefully will eliminate noise in pull requests concerning the final newline.

The first commit is the result of the following command (requires GNU versions of find and sed):

$ find . -type f -not -path "./.git/*" -not -path "./deps/*" -not -path "./out/*" -not -path "./tools/eslint/*" -not -path "./tools/gyp/*" -not -path "./tools/icu/*" -not -path "./tools/msvs/*" -not -path "*/node_modules/*" -not -path "*.min.js" -not -path "./test/fixtures/*" -iregex '\(js\|cc\|h\|d\|man\|manifest|\rc\|md\|mardown\|eslintrc\|py\|txt\|sh\|pl\|json\|pem\|key\|crt\|ini\|cnf\|html\|css\|makefile\|gyp\|gypi\|mk\|status\|xml\|gitignore\|r\|tmpl\|stp\|spec\|remarkrc\|rc\|mailmap\|inc\|gitattributes\|eslintignore\|editorconfig\|1\)$' | xargs sed -i -e '$a\'

The only offending file was benchmark/README.md. I've opted to not change test assets which would have required changes in a bunch of tests.

The second commit enables the editorconfig option. The pattern was written with the assumption that no one will manually edit files inside any node_modules directory or .min.js files.

@silverwind silverwind added the tools Issues and PRs related to the tools directory. label Nov 1, 2016
@silverwind silverwind mentioned this pull request Nov 2, 2016
Closed
@thefourtheye
Copy link
Contributor

So we are not doing eol-last, eslint rule?

@silverwind
Copy link
Contributor Author

silverwind commented Nov 2, 2016 •
edited
Loading

eol-last is already in effect, I totally missed that earlier.

@silverwind
Copy link
Contributor Author

Thanks, landed in dc88d19 and 15b83b9.

@silverwind silverwind closed this Nov 4, 2016
silverwind added a commit that referenced this pull request Nov 4, 2016
@silverwind
benchmark: add trailing newline for consistency
dc88d19 
PR-URL: #9410
Fixes: #9402
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Minwoo Jung <jmwsoft@gmail.com>
silverwind added a commit that referenced this pull request Nov 4, 2016
@silverwind
tools: enable final newline in .editorconfig
15b83b9 
PR-URL: #9410
Fixes: #9402
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Minwoo Jung <jmwsoft@gmail.com>
evanlucas pushed a commit that referenced this pull request Nov 7, 2016
@silverwind @evanlucas
benchmark: add trailing newline for consistency
dafdb7b 
PR-URL: #9410
Fixes: #9402
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Minwoo Jung <jmwsoft@gmail.com>
evanlucas pushed a commit that referenced this pull request Nov 7, 2016
@silverwind @evanlucas
tools: enable final newline in .editorconfig
aac4af2 
PR-URL: #9410
Fixes: #9402
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Minwoo Jung <jmwsoft@gmail.com>
@evanlucas evanlucas mentioned this pull request Nov 7, 2016
Merged
@MylesBorins
Copy link
Contributor

this will require manual backporting

silverwind added a commit to silverwind/node that referenced this pull request Nov 22, 2016
@silverwind
benchmark: add trailing newline for consistency
5243202 
PR-URL: nodejs#9410
Fixes: nodejs#9402
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Minwoo Jung <jmwsoft@gmail.com>
silverwind added a commit to silverwind/node that referenced this pull request Nov 22, 2016
@silverwind
tools: enable final newline in .editorconfig
c52e74d 
PR-URL: nodejs#9410
Fixes: nodejs#9402
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Minwoo Jung <jmwsoft@gmail.com>
@silverwind
Copy link
Contributor Author

silverwind commented Nov 22, 2016 •
edited
Loading

@thealphanerd you can cherry-pick just 15b83b9. The EOL issue on benchmark/README.md seems to not be present in v6.x, v6.x-staging and v4.x, so the first commit would be a empty one :)

If we're okay with empty commits, I could provide you one though.

MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
@silverwind @MylesBorins
tools: enable final newline in .editorconfig
a5d27f3 
PR-URL: #9410
Fixes: #9402
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Minwoo Jung <jmwsoft@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
@silverwind @MylesBorins
tools: enable final newline in .editorconfig
e42ee4e 
PR-URL: #9410
Fixes: #9402
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Minwoo Jung <jmwsoft@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
@silverwind @MylesBorins
tools: enable final newline in .editorconfig
5c6d908 
PR-URL: #9410
Fixes: #9402
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Minwoo Jung <jmwsoft@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
@silverwind @MylesBorins
tools: enable final newline in .editorconfig
459ec73 
PR-URL: #9410
Fixes: #9402
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Minwoo Jung <jmwsoft@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
@silverwind @MylesBorins
tools: enable final newline in .editorconfig
0e745e7 
PR-URL: #9410
Fixes: #9402
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Minwoo Jung <jmwsoft@gmail.com>
This was referenced Dec 21, 2016
Merged
Merged
@aliscco aliscco mentioned this pull request Jun 21, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gibfahn gibfahn gibfahn approved these changes

@JungMinu JungMinu JungMinu approved these changes

@lpinca lpinca lpinca approved these changes

@thefourtheye thefourtheye thefourtheye approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
tools Issues and PRs related to the tools directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@silverwind @thefourtheye @MylesBorins @jasnell @lpinca @JungMinu @gibfahn