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

add .editorconfig to set some basic coding styles #225

Merged
merged 2 commits into from
Mar 9, 2019
Merged

add .editorconfig to set some basic coding styles #225

merged 2 commits into from
Mar 9, 2019

Conversation

rudyhuyn
Copy link
Contributor

@rudyhuyn rudyhuyn commented Mar 9, 2019

To maintain consistent code style between contributors and simplify merging, the solution should provide a .editorconfig to set some basic rules (already used by the current code source).

  • use UTF-8 with BOM (some files are without)
  • use CR-LF
  • use Space to indent (sorry #teamTab)
  • trim trailing whitespace
  • be sure to add a new line at the end of files.

Documentation

@HowardWolosky HowardWolosky added No Behavior Change tooling Work that primarily impacts the yaml or administration of the GitHub project itself. labels Mar 9, 2019
@HowardWolosky
Copy link
Member

This is great. Thanks Rudy. We had just been discussing yesterday that specific issue about the BOM and our desire to find a way to help enforce it stay there. How timely.

@HowardWolosky HowardWolosky merged commit 140a5b3 into microsoft:master Mar 9, 2019
@janisozaur
Copy link
Contributor

I'm curious why BOM is preferred?

I also have concerns about explicitly stating crlf in some of the files, why is that needed? I would imagine this will clash with whatever is the default on the platform and respected by git. Additionally, what's the reason for selecting crlf instead of the more commonly supported lf?

@HowardWolosky
Copy link
Member

Reasonable questions, @janisozaur.

For BOM, it's because our team has now twice encountered a problem (once before public release, and again more recently in #197) where VSTest failed to correctly read embedded Unicode characters, and the failure reason wasn't immediately obvious. In this commit as part of #197, Brett made the change from the embedded Unicode character to using the escaped character sequence, but that was only necessary because the BOM got lost during his editing session.

So, I'll pose you the counter -- do you have any specific objection for ensuring the BOM is there?

As for crlf, this one I'm less set on. If you're concerned about it, I want to hear more. Please open an issue so that we can have a discussion with the community on the impact of the current decision, and whether we should keep crlf, change it to lf, or just remove the end_of_line option entirely.

Thanks for your diligence and interest!

@janisozaur
Copy link
Contributor

do you have any specific objection for ensuring the BOM is there?

I sometimes do things to sources when playing with them on various setups, that includes concatenating files. BOM gets in the way and most stuff already expects and assumes utf8, so there's little need for BOM. Some Windows users in the projects I contribute to switched some option in MSVC to behave like that or maybe it was a change that came with some recent MSVC update. Either way, there's no BOM in our utf8 files, no tool complains and I prefer it that way. Most tools I've seen prefer no BOM and it is often being switched from commit to commit, having no BOM in my experience leads to less churn. I have no idea what vstest is, so I can't comment on that.

My take on end_of_line is it should not be set, based on the presumption that git client already handles that. In the odd case where someone has misconfigured client and commits alien crlf, it should still be corrected in a future commit to git-native lf, so it can be correctly processed by others, contrary to a scenario where lf was chosen as default.

@HowardWolosky
Copy link
Member

vstest is the test execution harness that runs our UT's in our build pipeline.

I've opened #237 to allow for open discussion on the impact of specifying a default line ending for these files. Please join the discussion there, and once a decision has been reached within there, a PR (if necessary) can be created.

Thanks!

@rudyhuyn
Copy link
Contributor Author

rudyhuyn commented Mar 9, 2019

I'm ok with janisozaur, if we want to support Mac and Linux users, we can remove end_of_line = crlf

jlaanstra added a commit to jlaanstra/calculator that referenced this pull request Mar 11, 2019
mcooley pushed a commit that referenced this pull request Mar 12, 2019
* Remove duplicate .editorconfig
* Update .editorconfig to remove forcing crlf as discussed in #225.
chvalean added a commit to chvalean/LISAv2-1 that referenced this pull request Mar 12, 2019
To maintain consistent code style between contributors and simplify merging, the solution should provide a .editorconfig to set some basic rules (already used by the current code source).

- use UTF-8
- use Tab to indent (as defined in https://github.com/LIS/LISAv2/blob/master/Documents/How-to-use.md)
- trim trailing white-space
- add a new line at the end of files for bash scripts.

References:
- microsoft/calculator#225
- https://editorconfig.org/
HowardWolosky pushed a commit that referenced this pull request Mar 13, 2019
#225 accidentally added a duplicate .editorconfig to the repo as well as to the solution.
#257 removed the duplicate .editorconfig, but not its entry in the solution.
This update removes the extra .editorconfig reference to a version that #257 already removed from the repo.
ader1990 pushed a commit to ader1990/LISAv2-1 that referenced this pull request Apr 11, 2019
To maintain consistent code style between contributors and simplify merging, the solution should provide a .editorconfig to set some basic rules (already used by the current code source).

- use UTF-8
- use Tab to indent (as defined in https://github.com/LIS/LISAv2/blob/master/Documents/How-to-use.md)
- trim trailing white-space
- add a new line at the end of files for bash scripts.

References:
- microsoft/calculator#225
- https://editorconfig.org/
ader1990 pushed a commit to ader1990/LISAv2-1 that referenced this pull request Apr 11, 2019
To maintain consistent code style between contributors and simplify merging, the solution should provide a .editorconfig to set some basic rules (already used by the current code source).

- use UTF-8
- use Tab to indent (as defined in https://github.com/LIS/LISAv2/blob/master/Documents/How-to-use.md)
- trim trailing white-space
- add a new line at the end of files for bash scripts.

References:
- microsoft/calculator#225
- https://editorconfig.org/
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tooling Work that primarily impacts the yaml or administration of the GitHub project itself.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants