-
Notifications
You must be signed in to change notification settings - Fork 50
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
Add .editorconfig and .gitattributes #61
Conversation
Thanks, I'll let @domenic confirm since he's a little more familiar with these. |
end_of_line = lf | ||
insert_final_newline = true | ||
charset = utf-8 | ||
indent_size = 1 |
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.
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.
Yeah. Currently, most (if not all) HTML files use 1 space only; JavaScript files and inline <script>
elements use 2 spaces; .htaccess
files are not indented consistently. And I don't think we need to worry about the indentation of SVG files in this case.
I'll wait for Domenic's reply and adjust the PR if necessary.
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.
1 space is preferred for HTML (or SVG). I guess to be consistent we should also do it for CSS and script and .htaccess, especially since we don't use file extensions so differentiating would be tricky (see #8).
I think we should fix those files to be 1 space while merging this if you're up for it. Otherwise I think adding .editorconfig will just cause confusion since it will cause peoples' text editors to start being inconsistent with the indentation inside a file.
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.
Can we have two spaces for .js ?
These have no indentation (I guess that's fine): This one has weird indentation (I suppose it's copy-pasted from an inline script): These CSS files use 2 spaces: Maybe there are other files that don't follow specified convention; I only checked manually. Maybe we can leave those alone until there's some reason to change them? |
My main concern is not having .editorconfig contradict the indentation we want. I.e. if we ever edit those files, we should not be fighting our editor which insists on 1 or 2 spaces per .editorconfig. I think the best solution for that is to consistify things. But an alternate solution would be to add a lot of exclude rules. Or switch to a safelist approach where we only add indent settings for certain known-good files. |
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.
Changing the review status of this per @domenic's feedback. Makes it clearer in various PR overviews this is not ready to be merged.
Closing due to inactivity. This would still be welcome, but probably needs to be more tightly scoped. |
Mostly stolen from the Streams repo.
Fix #18.