Opened 3 years ago
Closed 3 years ago
#28839 closed enhancement (fixed)
adding a minimal lgtm.yml file
Reported by: | chapoton | Owned by: | |
---|---|---|---|
Priority: | minor | Milestone: | sage-9.0 |
Component: | scripts | Keywords: | |
Cc: | tscrim, fbissey, embray, vbraun | Merged in: | |
Authors: | Frédéric Chapoton | Reviewers: | Erik Bray |
Report Upstream: | N/A | Work issues: | |
Branch: | f3640c2 (Commits, GitHub, GitLab) | Commit: | f3640c29b6adcbeed6a5e6c0ac5e89a2705966d7 |
Dependencies: | Stopgaps: |
Description (last modified by )
This will tell LGTM to use Python3 during its code analysis.
as suggested here:
https://lgtm.com/help/lgtm/analysis-faqs#how-python-version-identified
Change History (20)
comment:1 Changed 3 years ago by
Branch: | → u/chapoton/28839 |
---|---|
Commit: | → d6728ff3c4512a30e14238e831325ab383a5413e |
Status: | new → needs_review |
comment:2 Changed 3 years ago by
Description: | modified (diff) |
---|
comment:4 Changed 3 years ago by
The reports can be found here:
https://lgtm.com/projects/g/sagemath/sage/?mode=list
but the problem is that it only runs when "master" branch is updated..
comment:5 follow-up: 7 Changed 3 years ago by
I wonder if there is a setting to change the branch to track.
comment:6 Changed 3 years ago by
the only way seems to be : change our main branch on github to be "develop"
comment:7 Changed 3 years ago by
Replying to embray:
I wonder if there is a setting to change the branch to track.
ISTM that LGTM is reading from the GitHub mirror of our repo, so I just need to change its default branch to the develop branch. I don't *think* there will be any consequences to doing so, so I'll go ahead and do that.
comment:8 Changed 3 years ago by
I'm looking at this site, and while many of the alerts look useful, a great many of the "errors" reported are due to supposedly incorrect arguments to a class's __init__
, except their analysis is not clever enough to consider classes which have a metaclass with a custom __call__
(e.g. our ClasscallMetaclass
which is used all over the place).
So perhaps while we're adding this file it might be helpful to globally exclude this and related errors as explained here. For example, from what I'm seeing in the errors, we want at least:
queries: - exclude: py/call/wrong-named-class-argument - exclude: py/call/wrong-number-class-arguments
Of course, we can also suppress alerts on individual lines, but these two are so pervasive it would not be practical. Suppressing them globally unfortunately could hide legitimate errors, though these errors are likely to be caught by tests in most cases.
comment:9 Changed 3 years ago by
Commit: | d6728ff3c4512a30e14238e831325ab383a5413e → 84f4cd3417d6ae6d94afcc99606309b32cb4ddfc |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
84f4cd3 | more stuff in lgtm.yml
|
comment:11 Changed 3 years ago by
Heh, actually it turns out the actual majority of errors are of the class py/unsafe-cyclic-import. We should probably globally disable that one as well :)
comment:12 follow-up: 15 Changed 3 years ago by
If you add that too, can you please also include in your commit message a link to this discussion so that it's clearer why it was added.
comment:13 Changed 3 years ago by
Commit: | 84f4cd3417d6ae6d94afcc99606309b32cb4ddfc → 7b76a05abf7f38575dfb0a13097370051b14610f |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
7b76a05 | more stuff in lgtm.yml
|
comment:14 Changed 3 years ago by
comment:15 Changed 3 years ago by
Reviewers: | → Erik Bray |
---|---|
Status: | needs_review → positive_review |
Replying to embray:
If you add that too, can you please also include in your commit message a link to this discussion so that it's clearer why it was added.
LGTM :)
Though please consider either squashing your commits and/or adding some more details or link to this ticket. "adding stuff to this file for no apparent reason"-type commit messages are not helpful :(
comment:16 Changed 3 years ago by
Commit: | 7b76a05abf7f38575dfb0a13097370051b14610f → f3640c29b6adcbeed6a5e6c0ac5e89a2705966d7 |
---|---|
Status: | positive_review → needs_review |
Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. This was a forced push. New commits:
f3640c2 | adding a minimal .lgtm.yml file
|
comment:18 Changed 3 years ago by
Status: | needs_review → positive_review |
---|
comment:20 Changed 3 years ago by
Branch: | u/chapoton/28839 → f3640c29b6adcbeed6a5e6c0ac5e89a2705966d7 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
New commits:
adding a .lgtm.yml file to specify the python version