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 auto code-review checks: tabs, trailing whitespace, etc. #12

Closed
dilijev opened this issue Jan 6, 2016 · 3 comments
Closed

Add auto code-review checks: tabs, trailing whitespace, etc. #12

dilijev opened this issue Jan 6, 2016 · 3 comments

Comments

@dilijev
Copy link
Contributor

dilijev commented Jan 6, 2016

@tcare has suggested adding an auto-code-review service via chakrabot account for this type of issue rather than blocking PRs on non-functional issues.

Details:

We don't want to block builds by having an extra check for style. What we would like to have instead is an auto code-review bot that will make comments about that sort of thing, and which the author has the freedom to ignore if necessary. Not all such style changes are worth making, and it's difficult to make a good logic for style that should always be applied.

For anything that would potentially be a build-breaking check (which would require almost unanimous agreement on the team), I would prioritize like this based on likelihood of consensus:

  1. Tabs
  2. Trailing whitespace
  3. Blank line at end of file
  4. Other style (very unlikely to get unanimous approval)

We already have some necessary style checks as build-breaking checks (where necessary pertains to environment compatibility or legal requirements):

  • [EOL Check] EOL characters must be checked into git as \n and not \r\n for platform compatibility.
  • [Copyright Check] The Copyright header on each file must match the header required by Microsoft legal.

Labeled as "Accepting PRs" because maybe the community knows of a tool we could deploy to help us with this. It might not be a change that makes it into this repo's source but it might still be helpful.

@dilijev
Copy link
Contributor Author

dilijev commented Mar 8, 2016

See comments in #473
I'll copy relevant content into this issue.

@dilijev
Copy link
Contributor Author

dilijev commented Mar 8, 2016

I had an OCD moment on tabs recently and checked myself: #425. It's not worth the time to bring in a small change like that. At some point, say around the time we implement an auto code-review system, it might be appropriate to do a pass over the whole code base and fix issues that such a system would flag.

@dilijev dilijev changed the title Add check for accidental introduction of tabs Add auto code-review checks: tabs, trailing whitespace, etc. Mar 8, 2016
@EdMaurer
Copy link
Contributor

EdMaurer commented May 3, 2016

Investment in whitespace tooling is below the bar at this point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants