Skip to content
This repository has been archived by the owner on Sep 19, 2020. It is now read-only.

ignore multiple rules + ~ syntax #119

Closed
wants to merge 3 commits into from
Closed

Conversation

grosser
Copy link
Contributor

@grosser grosser commented Mar 17, 2013

@grosser
Copy link
Contributor Author

grosser commented Mar 17, 2013

I'd say 99% of cases is that 1 line violates 1 rule. Even if 1 line violates more then 1 rule I would consider it a bad practice to just ignore all (including future) rules. Writing down all the rules will also make it easier for someone to just go in there and google what the hell FCXYZ is and maybe see it's describing something that's really bad. It also makes search easy if you want to fix all things that use search / invalid modes etc.

@acrmp
Copy link
Member

acrmp commented Mar 18, 2013

There are examples of the types of expressions in the choose rules to apply feature. It covers logical ANDs but I don't think these are useful here so I retract the FUD about more complex expressions. It would be good to avoid having two implementations of tags though. I'd be open to losing the dependency on gherkin where we lift the tag support from as an alternative.

@acrmp
Copy link
Member

acrmp commented Mar 18, 2013

The regression check says that your figure of 99% is not far off. I agree that we should encourage use of rule codes rather than other tags with this feature. On the other hand as a user having syntax that purports to be the same as the CLI but doesn't work when I try to use it in the same way would be really annoying.

@grosser
Copy link
Contributor Author

grosser commented Mar 18, 2013

I think it would be great to get a release out and see if anyone complains,
99% won't need complex expressions and of the 1% left maybe 10% know how to
do complex expressions :)
Maybe pick to an old branch and do 1.xxx ...

On Mon, Mar 18, 2013 at 4:12 PM, Andrew Crump notifications@github.comwrote:

The regression checkhttps://github.com/acrmp/foodcritic/blob/e1a232a11ddf1beae2ba601eaf7a972cded918fe/spec/regression/expected-output.txtsays that your figure of 99% is not far off. I agree that we should
encourage use of rule codes rather than other tags with this feature. On
the other hand as a user having syntax that purports to be the same as the
CLI but doesn't work when I try to use it in the same way would be really
annoying.


Reply to this email directly or view it on GitHubhttps://github.com//pull/119#issuecomment-15088194
.

acrmp pushed a commit that referenced this pull request Mar 19, 2013
acrmp pushed a commit that referenced this pull request Mar 19, 2013
@acrmp
Copy link
Member

acrmp commented Mar 19, 2013

Hi Michael,

I merged your changes but also ported them to use the same expression support as the cli in 99c57fa.

Thanks,

Andrew.

@acrmp acrmp mentioned this pull request Mar 19, 2013
@acrmp
Copy link
Member

acrmp commented Mar 19, 2013

Closing. Thanks Michael!

@acrmp acrmp closed this Mar 19, 2013
@acrmp
Copy link
Member

acrmp commented Mar 25, 2013

This has been released in 2.0.0.

It looks like we may not be ignoring properly where rules act on the whole cookbook using the cookbook keyword in foodcritic. The most obvious example of this is FC019.

acrmp pushed a commit that referenced this pull request Mar 25, 2013
@JeanMertz
Copy link

Should it be possible to add the ignore syntax above the line to be ignored? Enforcing 80 character rule, having a long string might make it more readable to put this above the actual line:

# we should ignore this during compile time
if node['ec2'] && !node['chef']['server']['elastic_ip'].empty? # ~FC023
  aws_elastic_ip 'chef server' do
    ...
  end
end

vs:

# ~FC023 -- we should ignore this during compile time
if node['ec2'] && !node['chef']['server']['elastic_ip'].empty?
  aws_elastic_ip 'chef server' do
    ...
  end
end

@emoshaya
Copy link

emoshaya commented May 1, 2015

I got this warning:
FC015: Consider converting definition to a LWRP: ./definitions/default.rb:1
Adding "# ~FC015" to the first line in the definitions/default.rb file did not ignore the warning...

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants