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

GitHub: PR system should enforce ASCII and whitespace conventions #141

Closed
StephanTLavavej opened this issue Sep 27, 2019 · 0 comments · Fixed by #229
Closed

GitHub: PR system should enforce ASCII and whitespace conventions #141

StephanTLavavej opened this issue Sep 27, 2019 · 0 comments · Fixed by #229
Labels
fixed Something works now, yay! infrastructure Related to repository automation

Comments

@StephanTLavavej
Copy link
Member

StephanTLavavej commented Sep 27, 2019

clang-format enforcement (#88 resolved by #132) is amazing, but I anticipate the need for more enforcement. Specifically:

  • We have a convention of requiring all sources to be ASCII. It's easy to accidentally introduce non-ASCII characters (e.g. Standardese often contains smart quotes and long dashes).
  • We avoid tabs in most files. It's currently possible for tabs to sneak into // clang-format off [...] // clang-format on regions.
  • We have whitespace conventions equivalent to VSCode's
"files.insertFinalNewline": true,
"files.trimFinalNewlines": true,
"files.trimTrailingWhitespace": true,

clang-format doesn't enforce the final newline conventions. It does trim trailing whitespace, but only in source files; other text files aren't covered. (And again, trailing whitespace is a concern in regions where we've disabled clang-format.)

  • We're using CRLF line endings. Nothing is preventing files with LF line endings from being introduced.
  • (optional) We have a convention of wrapping to 120 columns. clang-format does this in source files only, but it's imperfect (we've encountered situations where it'll happily exceed 120).

I've written a fairly simple file scanner that detects many of these issues; I could clean it up for production use. Of course, it's a C++ program that has to be compiled.

@StephanTLavavej StephanTLavavej added enhancement Something can be improved infrastructure Related to repository automation labels Sep 27, 2019
StephanTLavavej added a commit that referenced this issue Oct 30, 2019
Fixes #141.

* Change LF to CRLF.
* Remove UTF-8 BOMs.
* Add validate.cpp.
* Add validate to CMakeLists.txt.
* Add validate to azure-devops.
* Wrap enforce-clang-format.cmd.
* Code review feedback: abort().
* Code review feedback: Assign previous3.
* Code review feedback: Don't catch everything.
* Code review feedback: MaxErrorsForDisallowedCharacters.
* Cleanup: Remove `enabled: true`.
* Code review feedback: Character code comments.
* Code review feedback: Allow certain files to be tabby.
* Code review feedback: Use native wchar_t paths.
@StephanTLavavej StephanTLavavej added fixed Something works now, yay! and removed work in progress labels Oct 30, 2019
@StephanTLavavej StephanTLavavej removed the enhancement Something can be improved label Feb 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fixed Something works now, yay! infrastructure Related to repository automation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant