ETSY005 AST Corner Cases #3

Closed
joewilliams opened this Issue Jul 26, 2012 · 2 comments

Projects

None yet

3 participants

@joewilliams

This rule does not seem to detect that the following two notification lines are effectively the same thing:

notifies(:restart, resources(:service => "riak"))

notifies :restart, resources(:service => "riak")

The latter is detected by the rule, the former is not.

Additionally something like the following is not detected:

notifies :restart, "service[openvpn]"

Additionally if the service name happens to be a variable it won't detect it, example:

notifies :restart, resources(:service => riak)

@acrmp

Hi guys,

Just to put my 2p in. If you are using foodcritic 1.4.0+ this rule should be able to be rephrased as:

find_resources(ast).select do |resource|
  notifications(resource).any? do |notification|
    @coreservices.include?(notification[:resource_name]) and
      notification[:action] == :restart
  end
end

While it's more readable the notifications method also fails to catch your first example. We should fix that.

It will match the third example using new-style notifications though. In general foodcritic rules won't resolve variable values as foodcritic is performing static analysis.

Cheers,

Andrew.

@acrmp acrmp added a commit to acrmp/foodcritic that referenced this issue Jul 28, 2012
@acrmp acrmp Match notifications with parentheses. f4e58a1
@jonlives

Closing, changed ETSY005 to @acrmp's suggestion

@jonlives jonlives closed this Feb 1, 2013
@ruizink ruizink pushed a commit to ruizink/etsy-foodcritic-rules that referenced this issue Sep 20, 2014
Mário Santos Fix issue #3 by creating a new Nokogiri::XML before doing xpath query. 235784e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment