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

adding a minimal lgtm.yml file #28839

Closed
fchapoton opened this issue Dec 4, 2019 · 25 comments
Closed

adding a minimal lgtm.yml file #28839

fchapoton opened this issue Dec 4, 2019 · 25 comments

Comments

@fchapoton
Copy link
Contributor

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

CC: @tscrim @kiwifb @embray @vbraun

Component: scripts

Author: Frédéric Chapoton

Branch/Commit: f3640c2

Reviewer: Erik Bray

Issue created by migration from https://trac.sagemath.org/ticket/28839

@fchapoton fchapoton added this to the sage-9.0 milestone Dec 4, 2019
@fchapoton
Copy link
Contributor Author

Branch: u/chapoton/28839

@fchapoton
Copy link
Contributor Author

New commits:

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

@fchapoton
Copy link
Contributor Author

Commit: d6728ff

@fchapoton

This comment has been minimized.

@kiwifb
Copy link
Member

kiwifb commented Dec 4, 2019

comment:3

That looks interesting.

@fchapoton
Copy link
Contributor Author

comment:4

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..

@embray
Copy link
Contributor

embray commented Dec 4, 2019

comment:5

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

@fchapoton
Copy link
Contributor Author

comment:6

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

@embray
Copy link
Contributor

embray commented Dec 4, 2019

comment:7

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.

@embray
Copy link
Contributor

embray commented Dec 4, 2019

comment:8

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 4, 2019

Changed commit from d6728ff to 84f4cd3

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 4, 2019

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

84f4cd3more stuff in lgtm.yml

@fchapoton
Copy link
Contributor Author

comment:10

indeed. Done

@embray
Copy link
Contributor

embray commented Dec 4, 2019

comment:11

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 :)

@embray
Copy link
Contributor

embray commented Dec 4, 2019

comment:12

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 4, 2019

Changed commit from 84f4cd3 to 7b76a05

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 4, 2019

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

7b76a05more stuff in lgtm.yml

@embray
Copy link
Contributor

embray commented Dec 4, 2019

comment:14

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

@embray
Copy link
Contributor

embray commented Dec 4, 2019

comment:15

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 :(

@embray
Copy link
Contributor

embray commented Dec 4, 2019

Reviewer: Erik Bray

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 4, 2019

Changed commit from 7b76a05 to f3640c2

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 4, 2019

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

@sagetrac-git sagetrac-git mannequin removed the s: positive review label Dec 4, 2019
@sagetrac-git sagetrac-git mannequin added the s: needs review label Dec 4, 2019
@fchapoton
Copy link
Contributor Author

comment:17

ok, better like that ?

@embray
Copy link
Contributor

embray commented Dec 5, 2019

comment:19

Thank you!

@vbraun
Copy link
Member

vbraun commented Dec 8, 2019

Changed branch from u/chapoton/28839 to f3640c2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants