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:

GitHub link to the corresponding issue

Description (last modified by chapoton)

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 chapoton

Branch: u/chapoton/28839
Commit: d6728ff3c4512a30e14238e831325ab383a5413e
Status: newneeds_review

New commits:

d6728ffadding a .lgtm.yml file to specify the python version

comment:2 Changed 3 years ago by chapoton

Description: modified (diff)

comment:3 Changed 3 years ago by fbissey

Cc: vbraun added; braun removed

That looks interesting.

comment:4 Changed 3 years ago by chapoton

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 Changed 3 years ago by embray

I wonder if there is a setting to change the branch to track.

comment:6 Changed 3 years ago by chapoton

the only way seems to be : change our main branch on github to be "develop"

comment:7 in reply to:  5 Changed 3 years ago by embray

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 embray

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 git

Commit: d6728ff3c4512a30e14238e831325ab383a5413e84f4cd3417d6ae6d94afcc99606309b32cb4ddfc

Branch pushed to git repo; I updated commit sha1. New commits:

84f4cd3more stuff in lgtm.yml

comment:10 Changed 3 years ago by chapoton

indeed. Done

comment:11 Changed 3 years ago by embray

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 Changed 3 years ago by 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.

comment:13 Changed 3 years ago by git

Commit: 84f4cd3417d6ae6d94afcc99606309b32cb4ddfc7b76a05abf7f38575dfb0a13097370051b14610f

Branch pushed to git repo; I updated commit sha1. New commits:

7b76a05more stuff in lgtm.yml

comment:14 Changed 3 years ago by embray

Turns out there was already a recent issue for false positives due to custom metaclass __call__s. I added some examples to it.


New commits:

7b76a05more stuff in lgtm.yml

comment:15 in reply to:  12 Changed 3 years ago by embray

Reviewers: Erik Bray
Status: needs_reviewpositive_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 :(

Last edited 3 years ago by embray (previous) (diff)

comment:16 Changed 3 years ago by git

Commit: 7b76a05abf7f38575dfb0a13097370051b14610ff3640c29b6adcbeed6a5e6c0ac5e89a2705966d7
Status: positive_reviewneeds_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. This was a forced push. New commits:

f3640c2adding a minimal .lgtm.yml file

comment:17 Changed 3 years ago by chapoton

ok, better like that ?

comment:18 Changed 3 years ago by chapoton

Status: needs_reviewpositive_review

comment:19 Changed 3 years ago by embray

Thank you!

comment:20 Changed 3 years ago by vbraun

Branch: u/chapoton/28839f3640c29b6adcbeed6a5e6c0ac5e89a2705966d7
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.