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

lgtm: Exclude FIXME comments from alerts #3876

Closed
wants to merge 1 commit into from

Conversation

@danielgustafsson
Copy link
Member

commented May 12, 2019

LGTM triggers on FIXME comments issuing alerts for them, which is a bit overzealous as they clutter up the alert list and risk hiding more interesting things from alert-fatigue. Fix by hiding the fixme alerts to let us focus on the more interesting ones.

I don't really know if this does what it says on the tin, since I don't know LGTM at all, but the documentation at least implies that it will.

LGTM triggers on FIXME comments issuing alerts for them, which is a
bit overzealous as they clutter up the alert list and risk hiding
more interesting things from alert-fatigue. Fix by hiding the fixme
alerts to let us focus on the more interesting ones.

Closes #xxxx
@jay

This comment has been minimized.

Copy link
Member

commented May 13, 2019

I'm not a fan of FIXMEs they seem to hang around indefinitely. I think we should discourage them. I'm not sure if your commit stops that from happening.

@bagder

This comment has been minimized.

Copy link
Member

commented May 13, 2019

I'm rather indifferent. I don't like FIXMEs in the code, but I don't think the lgtm warning makes us remove them... If we really want them removed we add a check to checksrc! =)

@danielgustafsson

This comment has been minimized.

Copy link
Member Author

commented May 13, 2019

I agree that FIXME's tend to never get addressed (in any codebase), so if we want to tackle that we should perhaps turn the ones we have into TODO items and add a checksrc check for them.

What I want to achieve with this is however to increase the signal-to-noise ratio in LGTM as they currently are the majority of all alerts (with another one being a false positive).

bagder added a commit that referenced this pull request May 14, 2019
They serve very little purpose and mostly just add noise. Most of them
have been around for a very long time. I read them all before removing
or rephrasing them.

Ref: #3876
bagder added a commit that referenced this pull request May 16, 2019
They serve very little purpose and mostly just add noise. Most of them
have been around for a very long time. I read them all before removing
or rephrasing them.

Ref: #3876
Closes #3883
@bagder

This comment has been minimized.

Copy link
Member

commented May 16, 2019

Maybe we can leave this rule in now when I've cleaned up the FIXME situation in the code...

@danielgustafsson

This comment has been minimized.

Copy link
Member Author

commented May 16, 2019

Maybe we can leave this rule in now when I've cleaned up the FIXME situation in the code...

Agreed, no need to exclude something which should no longer exist.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
3 participants
You can’t perform that action at this time.