-
Notifications
You must be signed in to change notification settings - Fork 1k
feat: the global syntax linter #34024
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
base: master
Are you sure you want to change the base?
Conversation
| and similarly for consecutive pairs of `open` or `section` and `end`. | ||
| -/ | ||
| public register_option linter.globalSyntax : Bool := { | ||
| defValue := true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default value should eventually be false. This linter should only run in CI and possibly on a schedule.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it expensive? I'd actually like this to be always on and in core
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not benchmarked it, but I think that it is probably quite cheap.
The main issue is that it is incompatible with file edits. Linters can only reliably act on a single command at a time, so any linter that acts based on more than one command is exploiting a hack, like this one. The hack in this case is the IO.Ref that the linter uses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My earlier comment stands: I don't think that syntax linters can reliably lint more than one command. However, I have in mind a different implementation of this linter that may be slightly more robust to file edits, but would inevitably require restarting a file to get reliable results.
PR summary ca7d6f198fImport changes exceeding 2%
|
| File | Base Count | Head Count | Change |
|---|---|---|---|
| Mathlib.Init | 46 | 48 | +2 (+4.35%) |
| Mathlib.Algebra.Ring.Units | 176 | 178 | +2 (+1.14%) |
| Mathlib.Data.ENNReal.Basic | 669 | 671 | +2 (+0.30%) |
| Mathlib.Data.Matrix.Basic | 842 | 844 | +2 (+0.24%) |
| Mathlib.Topology.Algebra.IsUniformGroup.Defs | 982 | 984 | +2 (+0.20%) |
| Mathlib.Analysis.Normed.Order.Basic | 1035 | 1037 | +2 (+0.19%) |
| Mathlib.Tactic | 2901 | 2903 | +2 (+0.07%) |
Import changes for all files
| Files | Import difference |
|---|---|
| There are 7440 files with changed transitive imports taking up over 319500 characters: this is too many to display! | |
You can run scripts/import_trans_difference.sh all locally to see the whole output. |
Declarations diff
+ RangesToKinds
+ RangesToKinds.insert
+ cancellingPairs
+ globalSyntaxLinter
+ startersEnders
+ substringOfRange
+ toggle
You can run this locally as follows
## summary with just the declaration names:
./scripts/declarations_diff.sh <optional_commit>
## more verbose report:
./scripts/declarations_diff.sh long <optional_commit>The doc-module for script/declarations_diff.sh contains some details about this script.
No changes to technical debt.
You can run this locally as
./scripts/technical-debt-metrics.sh pr_summary
- The
relativevalue is the weighted sum of the differences with weight given by the inverse of the current value of the statistic. - The
absolutevalue is therelativevalue divided by the total sum of the inverses of the current values (i.e. the weighted average of the differences).
|
This PR/issue depends on: |
| In this file, we define classes for fields and groups that are both normed and ordered. | ||
| These are mostly useful to avoid diamonds during type class inference. | ||
| -/ | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#34047 deprecates this module
|
By the way, let me also point out that I would like to have tests for this linter, but the current implementation does not allow |
Found by the linter at #34024.
Found by the linter at #34024.
The
globalSyntaxlinter emits a warning on pairs of consecutive commands with no overall effect.For instance, the linter would flag
and similarly for consecutive pairs of
openorsectionandend.