-
Notifications
You must be signed in to change notification settings - Fork 144
Add an .editorconfig
file
#212
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
Conversation
b792149
to
407bb64
Compare
Hold on, I think Markdown should have lines no longer than 80 characters. I'll do some more research on best practices there. |
2fa83c5
to
aa2051e
Compare
aa2051e
to
467fa3a
Compare
467fa3a
to
762f1d1
Compare
.editorconfig
Outdated
trim_trailing_whitespace = false | ||
|
||
[*.css] | ||
indent_style = tab |
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'm not quite sure about this. Currently, the CSS test files are indented with tabs, and so this setting would keep this tabs. Or we could (in a separate PR) convert the CSS files to also use spaces for consistency. I'd prefer the second option and would be okay with both. :-)
@westonruter @sabberworm What do you think?
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.
And should we allow (semi-)unlimited line lengths for the CSS files (as some files have compressed 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've looked through some more CSS files: Their indentation is are a colorful mixture of tabs, 2 spaces and 4 spaces. :-)
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.
Well, adding an editorconfig doesn’t mean the files need to be reformatted, it’s just a hint to the IDE on how such a file should be formatted.
I think it’s OK for fixtures to differ in indentation since the parser needs to be able to handle all styles found in the wild. Some fixtures are just real-life cases of files that the parser couldn’t handle correctly, which is why they’re committed as-is.
Personally I vastly prefer tab indentation over spaces but I prefer consistency even more so I don’t think we need to special-case *.css
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.
Okay, I've remove the CSS special case now and repushed.
This helps contributors get the basic formatting options in their IDEs automatically set right. (The setting for PHP files complies to the PSR-12 standard.) This change does not autoformat any files with these settings, though.
762f1d1
to
1e0e7e6
Compare
This helps contributors get the basic formatting options in their IDEs
automatically set right. (The setting for PHP files complies to the
PSR-12 standard.)
This change does not autoformat any files with these settings, though.