FC003 still complains when returning #92

Closed
sethvargo opened this Issue Dec 9, 2012 · 13 comments

Projects

None yet

4 participants

@sethvargo
Contributor

Given the following line:

return Chef::Log.warn("This recipe uses search. Chef Solo does not support search.") if Chef::Config[:solo]

I AM actually validating that this recipe requires Chef Solo and halting execution thereafter. FC still complains.

I really prefer this syntax because it prevents me from infinite nesting syndrome and looks cleaner.

@miketheman
Contributor

Personally, having the condition at the end of a longer line, especially one that contains text, lets me more likely gloss over it in code.

I totally get it, just saying I think I like the other one meself.

@sethvargo
Contributor

Even without the predicate, it still fails:

# still throws FC003
if Chef::Config[:solo]
  return Chef::Log.warn("...")
end

This works, but it adds an additional layer of nested:

if Chef::Config[:solo]
  Chef::Log.warn("...")
else
  # other code
end
@miketheman
Contributor

So I suspect what you're looking for is in here 0.
Seems like playing around with the conditions of the specific amount of descendant counts might be somewhere to start.
Especially since the code has a nice TODO above it. 😁

@sethvargo
Contributor

Thanks for finding that @miketheman. I hadn't had an opportunity to even fork it yet, but you saved me the hassle 😄

@miketheman
Contributor

Any time. If you were to git blame, you'd find yours truly was last at bat.

@acrmp
Owner
acrmp commented Mar 19, 2013

Hi guys,

I've pushed a fix for this to master. Can you test and let me know if this resolves the issue.

Thanks,

Andrew.

@miketheman
Contributor

Exploded.
ruby 1.9.3, chef (10.16.4) & FC from master, stack here: https://gist.github.com/miketheman/5219076

@acrmp
Owner
acrmp commented Mar 22, 2013

Hi Mike,

Thanks for trying to test.

This error is because we don't currently support pulling foodcritic in from a git source with bundler (see #114, #120).

Cheers,

Andrew.

@miketheman
Contributor

wat. https://github.com/acrmp/foodcritic/blob/master/lib/foodcritic/chef.rb#L45-L60

So regular gem installs will include this file, but the source code apparently does not? I'm a little confused.

@markjreed

I really think this check needs to be a lot less specific. Trying to figure out whether or not a given section of code is protected by the proper conditional is an official Hard Problem that I don't think FC needs to be tackling. If there's a previous conditional in the code path that looks at Chef::Config[:solo] at all, that should probably be sufficient...

@miketheman
Contributor

Considering the conditional in Seth's first example on this ticket, that's a little harder to catch.
However, with the new functionality of having FC ignore a specific rule on a specific line, you can probably prevent these as needed.

@acrmp
Owner
acrmp commented Mar 26, 2013

Hi Mike,

The JSON is generated during the foodcritic build (see comment in #114). I opted not to commit the JSON because generally committing generated files is bad form. I hadn't considered the bundler git source use case though.

Cheers,

Andrew.

@acrmp
Owner
acrmp commented Mar 26, 2013

Hi Mark,

I think right now we are just checking that a conditional references Chef::Config[:solo] within the whole AST.

For example the following will not match FC003 currently:

nodes = search(:node, "hostname:[* TO *]")
if Chef::Config[:solo]
  Chef::Log.warn("This recipe uses search. Chef Solo does not support search.")
end

We could do the next simplest thing and just compare line numbers of searches and matches.

Currently checks_for_chef_solo? enumerates a few different types of conditional - not exhaustively though. We would probably be safe to reimplement the method without looking for conditionals / branching at all?

ast.xpath('//*[self::aref or self::call]
  [descendant::*[self::ident or self::tstring_content]/@value="solo"]/
  const_path_ref/descendant::const').map{|c|c['value']} == %w{Chef Config}

Cheers,

Andrew.

@sethvargo sethvargo closed this Jan 12, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment