[INFRA] Add .lgtm.yml file for better usage of LGTM CI tool

[DimitriPapadopoulos]

Previous commit bb6065a from #853 does not work around the LGTM alert.

We attempt to silence the LGTM alert using an lgtm.yml file, and use standard Flake8 noqa suppression comments to document the issue at hand.

Fixes #864.

Links:

• https://lgtm.com/projects/g/bids-standard/bids-specification/snapshot/56824aa6b869bcbec1fb14c50e3771cfc0358af5/files/pdf_build_src/process_markdowns.py?sort=name&dir=ASC&mode=heatmap#xea83c650d9db5491:1
• LGTM.com - false positive - 'noqa' suppression comments github/codeql#6517

[sappelhoff]

fine with me :-) the additional lgtm.yml file is a bit ugly in the repo, but I can live with it. Thanks!

[DimitriPapadopoulos]

The alternative to lgtm.yml would be to use plain # noqa instead of #noqa: F401. I feel it's a bad idea to lower code quality and precision to please a buggy tool.

[sappelhoff]

The alternative to lgtm.yml would be to use plain # noqa instead of #noqa: F401. I feel it's a bad idea to lower code quality and precision to please a buggy tool.

I agree with you. the alternative that I see is to not care about the LGTM tool reports. Maybe someone else can offer their opinion here - can you tag some people you'd like to chime in?

[DimitriPapadopoulos]

The alternative is OK with me. It's just that a small part of the LGTM alerts is actually useful, so it would be useful to be able to disable (some of) the false positives and focus on real issues.

[sappelhoff]

@Remi-Gau what's your opinion? Add an lgtm.yml file to the repo in order to ignore false-positives from the LGTM tool ... or ignore LGTM reports / live with the false positives?

I tend towards just merging the PR now.

[DimitriPapadopoulos]

That said, I'm not certain any more that lgtm.yml can disable the alerts displayed in LGTM.com:
https://lgtm.com/projects/g/bids-standard/bids-specification/

The lgtm.yml file in pydicom/pydicom doesn't seem to be working:
https://lgtm.com/projects/g/pydicom/pydicom/

Will have to look into it in more detail.

[DimitriPapadopoulos]

Also, file can be renamed to a hidden file .lgtm.yml. Would you rather have a hidden file?

[Remi-Gau]

Ignoring false positive is the kind of implicit knowledge that might confuse new users / maintainers: it also leads experienced users / maintainers to not spot new true positive when they show up. So if we could avoid this it would be great, no?

[DimitriPapadopoulos]

The alternative would be to document the places where we suppress false positives. Like this:

import foobar  # lgtm[py/unused-import]

However, that means adding multiple suppression comments for multiple linters:

import foobar  # noqa: F401 # pylint: disable=unused-import # lgtm[py/unused-import] 

[DimitriPapadopoulos]

The other alternative is to keep false positives as is, of course. But for projects with dozens of alerts, it's not easy to spot real positives in a haystack of false positives. There's no tool to classify them as real and false positives in LGTM.

[DimitriPapadopoulos]

Will have to look into it in more detail.

It actually does work. I was just confused by the existence of two alerts with very similar names, see pydicom/pydicom@2ded1d2:

py/clear-text-logging-sensitive-data
py/clear-text-storage-sensitive-data

[sappelhoff]

Ok great, is it possible to include an explanatory comment in the lgtm.yml file that points to the tool (LGTM), the report, and the reason why this file is in the repo? Making the file hidden .lgtm.yml would be another welcome change.

I think after that we could merge, given Remi's opinion as well.

[DimitriPapadopoulos]

There's a comment that points to the documentation of the LGTM py/unused-import alert:
https://lgtm.com/rules/6770079/

[sappelhoff]

Thanks @DimitriPapadopoulos

[DimitriPapadopoulos]

Because Semmle has joined GitHub, LGTM.com will be deprecated and replaced by GitHub code scanning.

The next step for LGTM.com: GitHub code scanning!

As far as I can understand, in simple cases such as this one, automated pull requests will be created to help us migrate:

We will do our best to help migrate repositories that actively use LGTM.com to flag potential security issues in their pull requests. For those repositories, we will create pull requests that add a GitHub Actions workflow that runs code scanning.