Adding support for an ignore file. #57

Closed
wants to merge 2 commits into
from

Projects

None yet

5 participants

@dmerrick

I love foodcritic. It's an awesome linter and very useful to maintain a quality standard in a chef repo shared among many people.

Very rarely, we get a few foodcritic warnings that we wish to ignore... be it from issues we do not wish to fix (for whatever reason) or occasional false positives.

Since we use foodcritic as part of our continuous delivery pipeline, we don't want our build to fail for these warnings, but we don't want to ignore an entire class of rules. It was with this in mind that I added the ability to pass an "ignore file" via the command line. An example file looks like this:

## foodcritic ignore file

# omitted because refactoring this makes it LESS elegant
FC004: Use a service resource to start and stop services: cookbooks/runit/recipes/default.rb:22
# omitted because it's too short and specific to make a library
FC014: Consider extracting long ruby_block to library: cookbooks/mongodb/recipes/mms-agent.rb:37
# omitted because contains false positives
FC017: LWRP does not notify when updated: cookbooks/python/providers/pip.rb

It's basically the output of a foodcritic run. You can choose to ignore a warning on a specific line, or omit the line number to ignore the rule across the entire file (c.p. the last line in the above example). Comments and newlines are ignored as expected.

I believe that this is a practical change, simply because foodcritic is often used in build pipelines and it's suboptimal to have to make changes to "trick" foodcritic into ignoring false positives.

Also, I know it's not perfect, but I figured I'd share it with you guys to collect feedback and to see what it would take to get it into master.

Thanks!
-Dana

@travisbot

This pull request passes (merged ee701b0 into d84c095).

@acrmp
Owner
acrmp commented Aug 16, 2012

Hi Dana,

Thanks! This looks like it solves a common problem for people and avoids excluding rules en-masse.

I was initially minded to merge it but thinking about it I have a couple of concerns:

  • I like the fact that you can specify the rule without line numbers in the ignore file. This helps avoid the situation where you are vulnerable to cookbook edits changing the line offsets of matches. This does raise a situation where you intend to exclude a specific instance of a warning and unknowingly introduce and exclude another warning in a subsequent edit. @jaymzh raised a request for similar functionality ages ago in issue #10 but suggested an approach that would be more robust by declaring the ignore within the cookbook next to the offending line. The downside of this approach is that it looked more involved to implement.
    #10 (comment)
  • Another alternative (which feels cleaner) is to not add this functionality to foodcritic at all and instead to provide examples in the docs of using grep with patterns from a file to exclude. I can imagine wanting to exclude based on a substring or regular expression which would slightly complicate the foodcritic implementation but is easy if you just use grep. This is my preferred option at the moment but I'd like to explore if there are things we can't do with grep that would be better implemented in foodcritic.

It also occurs to me that we could add an option to print out a checksum for each match which would make grepping out individual matches less brittle. I'm not sure if this would be a popular feature though.

For reference there are a couple of things I'd change if we were to merge:

  • Add tests to verify that the matches in the ignore file are excluded correctly and that it copes with
    a missing or malformed ignore file. The include custom rules feature 859a6f3 has similar scenarios.
  • Modify the loading of the ignore file to allow foodcritic rules that start with a different prefix, to allow exclusion of custom rules and not just those that ship with foodcritic.

Sorry for the brain dump but thanks very much for the contribution and for sparking a conversation early. Thanks for taking the time to make foodcritic better.

Cheers,

Andrew.

@acrmp
Owner
acrmp commented Aug 16, 2012

Ah. Sorry I just realised that you're specifically dealing with build failure which changes the situation a bit.

I'm going to sleep on it but would appreciate people's thoughts on this kind of functionality and the best place for it.

@dmerrick

Thanks so much for your reply. You make some great points.

Regardless of how it is implemented, I think the ability to ignore warnings is an important feature. I, too, considered adding in support for marking certain sections of code with a comment, but I decided to do it like this because it doesn't cruft up existing code, it doesn't require write access to the chef repo, and it was easier to implement.

At the moment, it simply omits specified warnings from the output, and does not affect the "epic fail" status. I was curious as to your thoughts on this; should my changes be refactored into FoodCritic::Linter.check(), or should I manually overwrite @is_failed in FoodCritic::Review? Either way, I could add a way for a user to pass another command line flag that determines if ignored warnings can affect the epic fail status, which could be very useful in separating the "I just don't want to see this in the output" and the more worrisome "I don't want this warning to fail my build anymore" use cases.

Another thing I considered was to build this into a second executable that acts as an output filter, so you could run something like foodcritic --whatever cookbooks | fc_ignore ignore.file and get more or less the same result. If you were morally against the idea of ignoring rules, this is the approach I would probably take.

Regarding tests: I'm totally on board with what you said. I started to write tests and it was taking me a while and I got to the point where I said "hmm, maybe I'll see if Mr. Foodcritic likes this idea before I keep going on this..."

Regarding loading the ignore file: great point! I didn't realize custom rules could have custom prefixes, so thanks so much for pointing it out.

Anyway, thanks for taking a look!

-Dana

@jaymzh
Collaborator
jaymzh commented Aug 21, 2012

So Andrew mentioned me having a similar desire... so I'll chime in.

I think my use-case is different. I use the command line options to set
everything to critical except a few rules we don't want to enforce. We set
that as a pre-commit, and you get a failure for the rules we feel strongly
about, and the rest will get printed out, but won't block your commit.

The reason I wanted ignores was for cases where I really want to do something
that we generally don't want to allow. There are a couple uses-cases:

  1. Bugs in Foodcritic. Over time we've seen false-positives in FC. Right now
    that means we wholly exempt that rule in our pre-commit. That sucks if it only
    triggers in a specific case that in my code I can just say:

{{{

Remove this ignore line after the XX release of FC

FCIGNORE: FC002

... do
...
end
}}}

  1. A use-case the FC rule hadn't thought of. A good example of this when
    extended the node attribute with new functions would make FC think you were
    accessing attributes in inconsistent ways. Or maybe I have some reason for not
    wanting a resource on the resource collection and thus need to wrap it in an
    "if" even though that's usually not the best approach. Or...

I don't want to ignore an entire rule, I just want to say "yo, this particular
block violates rule X and I'm OK with that."

If all you want is to ignore certain rules a command-line flag that takes a
list of rules not to process would be a lot easier and fit in with the already
existing command-line flags. You can already specify lists of rules to make
critical or not... why not just have one that sets them no alarm at all? (I
haven't looked at your code, fwiw, just the patch issue description and the
this email thread).

Phil Dibowitz phil@ipom.com
Open Source software and tech docs Insanity Palace of Metallica
http://www.phildev.net/ http://www.ipom.com/

"Be who you are and say what you feel, because those who mind don't matter
and those who matter don't mind."

  • Dr. Seuss
@acrmp
Owner
acrmp commented Aug 27, 2012

@jaymzh - I think Dana's pull request does handle the cases you have described. The ignore file specifies the file path and optionally line of the match to ignore.

As above relying on line numbers does have drawbacks but seems like the simplest thing (tm).

@jaymzh
Collaborator
jaymzh commented Aug 27, 2012

Ah I see. I haven't dug that deep into the code, but having a separate file
for the ignores isn't my preference, but it would do. Having the ignores
in-line has several advantages but the main two I can think of are (1) anyone
touching the code can see it's exempted and (hopefully) why (2) when the code
changes, you don't start ignoring random stuff because of bit-rot. Also, it's
more common amongst linters.

Nonetheless working, existing code is better non-existing code, so I won't
complain. I'm glad to have the feature in some form.

Phil Dibowitz phil@ipom.com
Open Source software and tech docs Insanity Palace of Metallica
http://www.phildev.net/ http://www.ipom.com/

"Be who you are and say what you feel, because those who mind don't matter
and those who matter don't mind."

  • Dr. Seuss
@acrmp
Owner
acrmp commented Sep 24, 2012

Hi Dana,

Sorry for the lack of feedback on your pull request!

I see you have made related changes recently on your fork for epic fail support, would it be possible to update this pull request?

Cheers,

Andrew.

@patcon
Contributor
patcon commented Oct 1, 2012

This convo is perfect guys 👍

Just wanted to chime in that I'm inclined to favor @jaymzh's inline approach. It would seem less brittle, and more coherent. The one downside is that there's no single file to review periodically (in case some are for pending bugfixes), but so long as the flagging string is unique, we can simply git grep FCIGNORE

@jaymzh
Collaborator
jaymzh commented Oct 1, 2012

Exactly. When someone's looking at the code I want it to be obvious something is exempted. It also then makes it clear where the comment would go since the code and the exemption are in one place. And yes, grep seems like the right way to find all instances.

@dmerrick
dmerrick commented Oct 9, 2012

Thanks so much to everyone for getting involved in the discussion. I knew that implementing some form of ignorefile support was an important feature to Foodcritic, so I went about it through the path of least resistance. We've been using my fork in my organization for the past few weeks and it has been suiting our needs (albeit on occasion causing us to update the line numbers due to code changes).

I agree with everyone in that the optimal solution would be some sort of inline "marking" that would cause Foodcritic to skip rule evaluation. Given the current implementation of Foodcritic's code parser, I suspect this will be a difficult fix. If I can help in any way, please let me know!

There are a couple more commits in my fork... fixing the epic-fail functionality and various changes to allow for a working gem package. I can add these changes into the pull request once I figure out how to add to an existing pull request without creating a new one :).

I still don't have tests... I'd made the judgement call that my code probably wasn't going to be merged into Foodcritic proper, so I opted not to take the time to finish them. I hope this doesn't prevent anyone from using my code!

Thanks again for taking the time to check out my work.

@patcon
Contributor
patcon commented Oct 10, 2012

Thanks @dmerrick. Couldn't hurt to share if you don't mind :)

I think you'll need to start a new PR, as I don't think it's possible to adjust the reference from a commit hash to a branch -- a branch is needed to have an evolving PR, I believe. Maybe create a new PR and reference this before closing?

@acrmp
Owner
acrmp commented Mar 31, 2013

Closing this as similar functionality was released in foodcritic 2.0.0.

Thanks to Dana for the original pull request and everyone involved for the quality discussion.

@acrmp acrmp closed this Mar 31, 2013
@dmerrick

Thanks for adding this in 2.0!

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