Skip to content

Conversation

@dgw
Copy link
Member

@dgw dgw commented Oct 1, 2021

Description

For whatever reason, when LGTM extracts code from this project, its interpretConfig tool (a Java JAR, the source for which I can't find) generates an effective config that tells the analysis to use Python 2 despite all of our tooling having been updated to drop it.

It would be nice to get rid of the false positive alerts we have about py2 stuff, so in the absence of any tooling (that I can find) to track down why LGTM infers the wrong Python version... Let's just set it.

Checklist

  • I have read CONTRIBUTING.md
  • I can and do license this contribution under the EFLv2
  • No issues are reported by make qa (runs make quality and make test)
    • No code changes
  • I have tested the functionality of the things this change touches
    • The only way to truly test this is to let LGTM analyze the code after merge, I believe. We've seen in the past that e.g. excluding alerts with inline comments won't take effect for PR builds, and the config file probably also behaves that way.

For whatever reason, when LGTM extracts code from this project, its
interpretConfig tool (a Java JAR, the source for which I can't find)
generates an effective config that tells the analysis to use Python 2
despite all of our tooling having been updated to drop that version.

It would be nice to get rid of the false positive alerts we have about
py2 stuff, so in the absence of any tooling (that I can find) to track
down why LGTM infers the wrong Python version... Let's just set it.
@dgw dgw added Build Housekeeping Code cleanup, removal of deprecated stuff, etc. labels Oct 1, 2021
@dgw dgw added this to the 8.0.0 milestone Oct 1, 2021
@dgw dgw requested a review from a team October 1, 2021 05:28
@Exirel
Copy link
Contributor

Exirel commented Oct 1, 2021

image

LGTM

@dgw
Copy link
Member Author

dgw commented Oct 1, 2021

Oh, the config change takes effect immediately during the PR build, nice. 3 new alerts, but 12 fixed? I can live with that.

@Exirel
Copy link
Contributor

Exirel commented Oct 1, 2021

🚢 🚢 🚢 🚢 🚢

@Exirel
Copy link
Contributor

Exirel commented Oct 18, 2021

@dgw maybe you could merge this one first since it touches LGTM and makes it better/faster?

@dgw dgw merged commit f937350 into master Oct 20, 2021
@dgw dgw deleted the lgtm-set-python-version branch October 20, 2021 04:39
@dgw
Copy link
Member Author

dgw commented Oct 20, 2021

maybe you could merge this one first since it touches LGTM and makes it better/faster?

idk about faster (though that check could hardly be slower 😂), but sure—it is better to run py3 analysis.

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

Labels

Build Housekeeping Code cleanup, removal of deprecated stuff, etc.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants