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
added editorconfig, fixed style exceptions #582
Conversation
|
||
# Unix-style newlines with a newline ending every file | ||
[*] | ||
end_of_line = lf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't that in conflict with the default OS end of line choice? Git already manages text files depending on the OS, checking in or out text files with LF or CRLF based on the context. I'd prefer the approach of adding file patterns in a .gitattributes file to specify which one should be considered as text, and leave the default OS mechanism.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's in conflict with windows default, which is the point. I'm not sure I see the advantage of adding .gitattributes when this line of the .editorconfig should do the trick. The final result is lf
in the repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not all editors have a plugin for .editorconfig, whereas the .gitattributes will be anyway be applied by git.
And it's more natural to allow the editor to edit the file in the native line ending.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally don't have a problem with adding this per se. It's fairly standard to add this to .editorconfig
, as you can see from a sampling of major oss projects here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no problem, anyway nobody in her right mind is developing on Windows.
charset = utf-8 | ||
|
||
# 2 space indentation | ||
[*.{js,json,yml,yaml,proto,sh,html,css}] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's also add our scripts without extension. I can only think about hack/build
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might be easier to just rename hack/build to hack/build.sh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one or the other is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree. Library sh and bash scripts should have the .sh
extension, but executable scripts should not. This is a subjective style issue, but I'm old school on this, I guess (even though I haven't always been 100% consistent myself). The only convenience for adding the extension has to do with using editors that require the extension for syntax highlighting. The Google Shell Style Guide agrees. Bottom line, if the file has a shebang line and the executable bit set, then don't add an extension. If the script is meant to be invoked by another script and is not executable standalone, then add the extension.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@generalhenry Looking good overall. My only request is to leave off the .sh
extension.
|
||
# Unix-style newlines with a newline ending every file | ||
[*] | ||
end_of_line = lf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally don't have a problem with adding this per se. It's fairly standard to add this to .editorconfig
, as you can see from a sampling of major oss projects here.
charset = utf-8 | ||
|
||
# 2 space indentation | ||
[*.{js,json,yml,yaml,proto,sh,html,css}] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree. Library sh and bash scripts should have the .sh
extension, but executable scripts should not. This is a subjective style issue, but I'm old school on this, I guess (even though I haven't always been 100% consistent myself). The only convenience for adding the extension has to do with using editors that require the extension for syntax highlighting. The Google Shell Style Guide agrees. Bottom line, if the file has a shebang line and the executable bit set, then don't add an extension. If the script is meant to be invoked by another script and is not executable standalone, then add the extension.
@hack/build $(AGENT) | ||
@hack/build $(LOGWORKER) | ||
@hack/build $(GATEWAY) | ||
@hack/build.sh $(CLI) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to not add .sh
. I realize it's a style issue, but I already commented my thoughts on this above. I know everyone isn't 100% consistent out there, including Docker itself. They add the extension under their hack
directory, and yet have dozens of shell scripts that don't under hack/make
. Bottom line, unless not having the extension is frustrating everyone with editors that key off the extension for syntax highlighting (I don't have that problem with vim), then let's standardize on leaving it off as I had it originally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I'll remove the commit at adds the extension.
042fabf
to
e4dab20
Compare
'.sh' reverted |
closes: #571
I tried my best to simply pave the cow paths. I did not add any rules for .md since we're not at all consistent.