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

Status badges

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 22 months ago by chapoton

  • Branch set to u/chapoton/28839
  • Commit set to d6728ff3c4512a30e14238e831325ab383a5413e
  • Status changed from new to needs_review

New commits:

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

comment:2 Changed 22 months ago by chapoton

  • Description modified (diff)

comment:3 Changed 22 months ago by fbissey

  • Cc vbraun added; braun removed

That looks interesting.

comment:4 Changed 22 months 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 follow-up: Changed 22 months ago by embray

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

comment:6 Changed 22 months 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 22 months 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 22 months 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 22 months ago by git

  • Commit changed from d6728ff3c4512a30e14238e831325ab383a5413e to 84f4cd3417d6ae6d94afcc99606309b32cb4ddfc

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

84f4cd3more stuff in lgtm.yml

comment:10 Changed 22 months ago by chapoton

indeed. Done

comment:11 Changed 22 months 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 follow-up: Changed 22 months 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 22 months ago by git

  • Commit changed from 84f4cd3417d6ae6d94afcc99606309b32cb4ddfc to 7b76a05abf7f38575dfb0a13097370051b14610f

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

7b76a05more stuff in lgtm.yml

comment:14 Changed 22 months 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 22 months ago by embray

  • Reviewers set to Erik Bray
  • Status changed from needs_review to 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 :(

Last edited 22 months ago by embray (previous) (diff)

comment:16 Changed 22 months ago by git

  • Commit changed from 7b76a05abf7f38575dfb0a13097370051b14610f to f3640c29b6adcbeed6a5e6c0ac5e89a2705966d7
  • Status changed from positive_review to 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:

f3640c2adding a minimal .lgtm.yml file

comment:17 Changed 22 months ago by chapoton

ok, better like that ?

comment:18 Changed 22 months ago by chapoton

  • Status changed from needs_review to positive_review

comment:19 Changed 22 months ago by embray

Thank you!

comment:20 Changed 22 months ago by vbraun

  • Branch changed from u/chapoton/28839 to f3640c29b6adcbeed6a5e6c0ac5e89a2705966d7
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.