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: Added .editorconfig file for basic encoding/indentation enforcement #2993

Closed
wants to merge 1 commit into from

Conversation

ronkorving
Copy link
Contributor

This helps editors (see http://editorconfig.org/#download) to apply the right indentation, end-of-line characters, and encoding rules to files of any type (extension) directly and without going through linters, etc. I can imagine some of the contents of this PR may need tweaking, so feedback is most appreciated.

@ronkorving ronkorving changed the title Added .editorconfig file for basic style enforcement Added .editorconfig file for basic encoding/indentation enforcement Sep 22, 2015
@mscdex mscdex added the meta Issues and PRs related to the general management of the project. label Sep 22, 2015
@@ -0,0 +1,22 @@
# See: http://EditorConfig.org
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd remove that comment

Copy link
Member

Choose a reason for hiding this comment

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

+1 on removing that

@silverwind
Copy link
Contributor

I'd add trim_trailing_whitespace for everything except markdown files.

@ronkorving
Copy link
Contributor Author

Why leave trailing whitespace in markdown files?

@sindresorhus
Copy link

@ronkorving
Copy link
Contributor Author

That is an absolutely horrible syntax. But sure.

@@ -0,0 +1,24 @@
root = true

[*]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm personally confused as to when to use * and **. In the hello world style example on http://editorconfig.org they show [*] as a sort of default setting for all files, but they also explain how * does not include slashes. So wouldn't that mean that this rule will match only files in the root dir? That would be a very odd default, so I must be mistaken. I just can't find an explanation for it.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's the thing that confuses me too about editorconfig. I think * is fine for a catch-all, but you should double check if the correct files match with the CLI tool.

Choose a reason for hiding this comment

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

@ronkorving this works because patterns without a slash are equivalent to **/pattern (essentially matching in the root directory and all child directories). If you need to match something specifically in the root, add an initial slash like /* (which will only match top-level files). We modeled this after the way globs work in .gitignore.

@reqshark
Copy link

does atom editor care about that file?

@ronkorving
Copy link
Contributor Author

@reqshark if you install the plugin for it, it will

@reqshark
Copy link

cool

@ronkorving
Copy link
Contributor Author

@reqshark I'm using Atom too, and that's the primary reason for wanting to see a .editorconfig file included in the project (since it doesn't have "project settings" or anything, which in a way is actually great).


[test/*.py]
indent_style = space
indent_size = 4
Copy link
Member

Choose a reason for hiding this comment

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

.py files have two space indent.

Copy link
Contributor

Choose a reason for hiding this comment

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

Might as well put the 2-space indent into * and use sections for files that differ.

Copy link
Member

Choose a reason for hiding this comment

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

+1. Can we match configure here as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jbergstroem configure's indentation is all over the place tbh, but I don't mind adding it.
2 space indent?

@silverwind
Copy link
Contributor

Another addition: vcbuild.bat should be crlf.

indent_style = space
indent_size = 4

[Makefile]
Copy link
Member

Choose a reason for hiding this comment

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

If you end up moving indent_size = 2 to *, can you add 8 here? (Ok, call me a unix beard but it's at least relatively default in make-land)

@silverwind
Copy link
Contributor

ping @ronkorving

@ronkorving
Copy link
Contributor Author

@silverwind perhaps *.bat should be CRLF? I've added that, but let me know if inappropriate or incomplete.

@ronkorving ronkorving force-pushed the editorconfig branch 2 times, most recently from 1905416 to 3a7bc86 Compare October 14, 2015 03:35
@silverwind
Copy link
Contributor

Hmm, not sure about that or if there even is a specific reason for CRLF in bat files, but we're somewhat inconsistent here thought most of these are in deps:

$ file $(find . -name "*.bat")
./deps/uv/docs/make.bat:                                  DOS batch file, ASCII text, with CRLF line terminators
./deps/uv/vcbuild.bat:                                    DOS batch file, ASCII text, with CRLF line terminators
./deps/npm/make.bat:                                      ASCII text
./deps/npm/node_modules/node-gyp/gyp/gyp.bat:             DOS batch file, ASCII text, with CRLF line terminators
./deps/npm/node_modules/node-gyp/gyp/samples/samples.bat: DOS batch file, ASCII text, with CRLF line terminators
./deps/openssl/openssl/ms/testss.bat:                     DOS batch file, ASCII text
./deps/openssl/openssl/ms/mingw32.bat:                    DOS batch file, ASCII text
./deps/openssl/openssl/ms/do_win64a.bat:                  ASCII text
./deps/openssl/openssl/ms/x86asm.bat:                     ASCII text
./deps/openssl/openssl/ms/tpem.bat:                       ASCII text
./deps/openssl/openssl/ms/mw.bat:                         DOS batch file, ASCII text
./deps/openssl/openssl/ms/testce.bat:                     DOS batch file, ASCII text
./deps/openssl/openssl/ms/test.bat:                       DOS batch file, ASCII text
./deps/openssl/openssl/ms/testssce.bat:                   ASCII text
./deps/openssl/openssl/ms/tencce.bat:                     ASCII text
./deps/openssl/openssl/ms/speed32.bat:                    ASCII text
./deps/openssl/openssl/ms/testpem.bat:                    DOS batch file, ASCII text
./deps/openssl/openssl/ms/do_nt.bat:                      ASCII text
./deps/openssl/openssl/ms/do_ms.bat:                      ASCII text
./deps/openssl/openssl/ms/do_win64i.bat:                  ASCII text
./deps/openssl/openssl/ms/testencce.bat:                  DOS batch file, ASCII text
./deps/openssl/openssl/ms/testenc.bat:                    DOS batch file, ASCII text
./deps/openssl/openssl/ms/tenc.bat:                       ASCII text
./deps/openssl/openssl/ms/32all.bat:                      ASCII text
./deps/openssl/openssl/ms/testpemce.bat:                  DOS batch file, ASCII text
./deps/openssl/openssl/ms/do_nasm.bat:                    ASCII text
./deps/openssl/openssl/ms/bcb4.bat:                       ASCII text
./deps/openssl/openssl/ms/testce2.bat:                    ASCII text
./deps/openssl/openssl/ms/tpemce.bat:                     ASCII text
./deps/openssl/openssl/engines/capierr.bat:               ASCII text
./deps/openssl/openssl/crypto/threads/win32.bat:          ASCII text
./deps/openssl/openssl/crypto/threads/ptest.bat:          ASCII text
./deps/openssl/openssl/crypto/threads/netware.bat:        DOS batch file, ASCII text
./deps/openssl/openssl/shlib/win32dll.bat:                ASCII text
./deps/openssl/openssl/shlib/win32.bat:                   ASCII text
./deps/openssl/openssl/Netware/build.bat:                 DOS batch file, ASCII text
./deps/openssl/openssl/Netware/cpy_tests.bat:             DOS batch file, ASCII text
./deps/openssl/openssl/Netware/set_env.bat:               DOS batch file, ASCII text
./deps/v8/tools/windows-tick-processor.bat:               DOS batch file, ASCII text
./deps/zlib/contrib/masmx86/bld_ml32.bat:                 ASCII text
./deps/zlib/contrib/masmx64/bld_ml64.bat:                 ASCII text
./tools/msvs/nodevars.bat:                                DOS batch file, ASCII text
./tools/gyp/gyp.bat:                                      DOS batch file, ASCII text, with CRLF line terminators
./tools/gyp/samples/samples.bat:                          DOS batch file, ASCII text, with CRLF line terminators
./vcbuild.bat:                                            DOS batch file, ASCII text, with very long lines, with CRLF line terminators

@jbergstroem
Copy link
Member

Looks like we'd want to exclude deps/ for the most part.

@silverwind
Copy link
Contributor

Looks *.bat has been attempted before in .gitattributes, but caused #330.

Would a line-ending conversion mess up git history?

@ronkorving
Copy link
Contributor Author

I've changed *.bat to vcbuild.bat
I think things should be good now?

end_of_line = crlf

[*.{md,markdown}]
trim_trailing_whitespace = false
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not trimming this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Someone mentioned before that end of line spaces can be used to enforce white lines iirc. I'm not a fan of the practice, but I think the only alternative to that is introducing <br> tags into the content.

Copy link
Contributor

Choose a reason for hiding this comment

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

If that is the case, I am okay with that. @silverwind

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, trailing space to force a line break in markdown. Not a fan of it either, but we should leave that option available to collaborators.

@silverwind
Copy link
Contributor

Is it possible to exclude deps/ somehow through a glob?

@ronkorving
Copy link
Contributor Author

@silverwind I'm not aware of any way to do this, especially in .editorconfig

@silverwind
Copy link
Contributor

tools should be the correct category, I'll fix that when landing.

This helps editors (see http://editorconfig.org/#download) to
apply the right indentation and encoding rules to files by
default.
@ronkorving
Copy link
Contributor Author

Prepended "tools: " ;)

@ronkorving ronkorving changed the title Added .editorconfig file for basic encoding/indentation enforcement tools: Added .editorconfig file for basic encoding/indentation enforcement Dec 16, 2015
@jbergstroem
Copy link
Member

@ronkorving all lowercase in commit title please (unless you reference acronyms, variables, classes, etc). Also, keep it shorter than 51 characters. There's a commit style-guide here.

@targos
Copy link
Member

targos commented Dec 16, 2015

We also usually write in the imperative form ("add" instead of "added") but it's not in the guide so I don't know if it's really a rule

@jbergstroem
Copy link
Member

@targos its in the extended guide we link to.

silverwind pushed a commit that referenced this pull request Dec 16, 2015
This helps editors (see http://editorconfig.org/#download) to
apply the right indentation and encoding rules to files by
default.

PR-URL: #2993
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Saúl Ibarra Corretgé <[email protected]>
@silverwind
Copy link
Contributor

Thanks! Landed in d63ccee with a conforming commit message. Please make sure to honor our commit message guidlines in the future. 😉

@silverwind silverwind closed this Dec 16, 2015
cjihrig pushed a commit that referenced this pull request Dec 16, 2015
This helps editors (see http://editorconfig.org/#download) to
apply the right indentation and encoding rules to files by
default.

PR-URL: #2993
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Saúl Ibarra Corretgé <[email protected]>
@rvagg rvagg mentioned this pull request Dec 17, 2015
MylesBorins pushed a commit that referenced this pull request Jan 19, 2016
This helps editors (see http://editorconfig.org/#download) to
apply the right indentation and encoding rules to files by
default.

PR-URL: #2993
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Saúl Ibarra Corretgé <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 19, 2016
This helps editors (see http://editorconfig.org/#download) to
apply the right indentation and encoding rules to files by
default.

PR-URL: #2993
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Saúl Ibarra Corretgé <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jan 19, 2016
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
This helps editors (see http://editorconfig.org/#download) to
apply the right indentation and encoding rules to files by
default.

PR-URL: nodejs#2993
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Saúl Ibarra Corretgé <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta Issues and PRs related to the general management of the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.