-
Notifications
You must be signed in to change notification settings - Fork 46
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
Linter #247
Labels
feature
New functionality, or change in existing functionality
infrastructure
Has to do with changes to the development process, e.g., build scripts, CI, testing utilities
Comments
Merged
maxitg
added a commit
that referenced
this issue
May 19, 2020
## Changes * This closes the C++ Style Refactor project. * Resolves #316. * Resolves #317. * Resolves #319. * Closes #357. * Reformats all C++ code according to [Google C++ Style Guide]. * Adds `.clang-format` file specifying the exceptions to the Google style guide. * Adds lint.sh script which prints a diff generated by `clang-format` (if any) and the errors generated by `cpplint`, and allows one to apply formatting in place with `./lint.sh -i`. * Adds Lint step to CI before Build. * Updates .gitbub/CONTRIBUTING.md with instructions on how to use lint.sh. * Adds the missing test file to the Xcode project. ## Comments * Linting in CI is not parallelized and is using the same Docker image because it will eventually include linting of the Wolfram Language code as well as described in #247. * Bin packing is now disallowed in addition to the existing style exceptions. * All `cpplint` errors are fixed as well, not just the formatting. * `#include <chrono>` is not allowed by `cpplint` for Chromium-specific reasons, so added `// NOLINT` there. * Renamed `SetTest.cpp` -> `Set_test.cpp` because the suffix appears to be hard-coded to `cpplint`. * Before the formatting, the code was mostly following this style (imperfectly): ``` --- Language: Cpp BasedOnStyle: Google ColumnLimit: 120 BinPackArguments: false BinPackParameters: false AllowShortFunctionsOnASingleLine: Empty IndentWidth: 4 NamespaceIndentation: All AccessModifierOffset: -4 FixNamespaceComments: false SpaceAfterTemplateKeyword: false BreakConstructorInitializers: AfterColon SpacesBeforeTrailingComments: 1 ... ``` ## Examples * Break formatting/code style somewhere in the code, and run `./lint.sh`. For example, we can remove `explicit` on line 22 of Match.cpp, and add a spurious space on the line below: ``` > ./lint.sh --- libSetReplace/Match.cpp +++ formatted @@ -24 +24 @@ - bool operator()(const MatchPtr& a, const MatchPtr& b) const { + bool operator()(const MatchPtr& a, const MatchPtr& b) const { libSetReplace/Match.cpp:22: Single-parameter constructors should be marked explicit. [runtime/explicit] [5] Done processing libSetReplace/Match.cpp Total errors found: 1 ``` * We can run `./lint.sh -i` to fix the space automatically: ``` > ./lint.sh -i --- libSetReplace/Match.cpp +++ formatted @@ -24 +24 @@ - bool operator()(const MatchPtr& a, const MatchPtr& b) const { + bool operator()(const MatchPtr& a, const MatchPtr& b) const { libSetReplace/Match.cpp:22: Single-parameter constructors should be marked explicit. [runtime/explicit] [5] Done processing libSetReplace/Match.cpp Total errors found: 1 ``` * The output is the same, however, if we run `./lint.sh` again, there will be no diff: ``` > ./lint.sh libSetReplace/Match.cpp:22: Single-parameter constructors should be marked explicit. [runtime/explicit] [5] Done processing libSetReplace/Match.cpp Total errors found: 1 ``` * Finally, if we put explicit back in, there will be no output, which means everything is ok: ``` > ./lint.sh ```
If we incorporate macros like |
May be relevant: https://github.com/WolframResearch/codeinspector |
maxitg
added
the
infrastructure
Has to do with changes to the development process, e.g., build scripts, CI, testing utilities
label
Dec 14, 2020
Merged
aokellermann
added a commit
that referenced
this issue
Dec 18, 2020
## Changes * Adds [markdown linter](https://github.com/DavidAnson/markdownlint) * Enforces 120 characters line width * Related to #247 * Closes #582 ## Examples Some common errors: ![image](https://user-images.githubusercontent.com/26678747/102560353-08b5a880-40a0-11eb-87be-be2787ccdc6c.png) <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/maxitg/setreplace/581) <!-- Reviewable:end -->
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
feature
New functionality, or change in existing functionality
infrastructure
Has to do with changes to the development process, e.g., build scripts, CI, testing utilities
The code consistency is currently somewhat out of whack. Tabs and spaces are used inconsistently, indentation is different, the line width is not enforced, etc.
We need a linter that would work along with CI and if not automatically fix then at least detect issues with code style and consistency.
Some lint tests we might want are:
Flatten
/ReplaceAll
/Thread
used without an explicit level spec (might need a different mechanism forThread
).Module
/With
/Block
:GeneralUtilities`
:FindDeadCode
.Linters we need:
The text was updated successfully, but these errors were encountered: