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

Linter #247

Open
3 of 12 tasks
maxitg opened this issue Mar 1, 2020 · 3 comments
Open
3 of 12 tasks

Linter #247

maxitg opened this issue Mar 1, 2020 · 3 comments
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

@maxitg
Copy link
Owner

maxitg commented Mar 1, 2020

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 for Thread).
  • Module/With/Block:
    • Missing definitions;
    • Unused definitions.
  • Static WL code analysis
  • Spacing issues
    • Enforce line width;
    • Check there are no tabs;
    • No spaces at the end of files;
    • No more than 2 spaces in between characters (unless followed by a comment);
    • No more than 2 empty lines at a time;
    • Empty lines present at the end of files.
  • Wolfram Language code formatting
    • Indentation;
    • Check there are either always or never semicolons at the end of definitions.
  • Check for broken links
  • Tools from GeneralUtilities` :
    • FindDeadCode.

Linters we need:

@maxitg maxitg added the feature New functionality, or change in existing functionality label Mar 1, 2020
@maxitg maxitg self-assigned this Mar 1, 2020
@maxitg maxitg removed their assignment May 14, 2020
@maxitg maxitg mentioned this issue May 18, 2020
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
```
@maxitg maxitg changed the title Linter Wolfram Language Linter Oct 15, 2020
@maxitg maxitg changed the title Wolfram Language Linter Linter Oct 15, 2020
@taliesinb
Copy link
Collaborator

If we incorporate macros like ModuleScope (see #461) then we should detect buried macros in the linter itself, since they don't evaluate properly (they do actually evaluate, but at runtime, which is quite slow).

@daneelsan
Copy link
Collaborator

May be relevant: https://github.com/WolframResearch/codeinspector

@daneelsan
Copy link
Collaborator

@maxitg maxitg added the infrastructure Has to do with changes to the development process, e.g., build scripts, CI, testing utilities label Dec 14, 2020
@maxitg maxitg assigned maxitg and unassigned maxitg Dec 14, 2020
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
Projects
None yet
Development

No branches or pull requests

3 participants