Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Incorrect FC019 alarms #22

Closed
jaymzh opened this Issue · 27 comments

2 participants

@jaymzh
Collaborator

doing:

node['foo'].attribute?('waka')

will get you an FC019 alarm which I don't think is valid. :(

@acrmp
Owner

Hi Phil,

I can't reproduce the warning from the example given (using the current version of the gem). Are you able to share a more complete example?

I need to push another release soon as there are some other fixes on master it would be good to make more widely available. If we can get this one in there as well that would be great.

Thanks,

Andrew.

@jaymzh
Collaborator
if node['something']['bar'] || node.in_tier?('foof')

Where in_tier? is a library function.

@acrmp
Owner

Ah. Foodcritic knows that in_tier? is not a built-in method on node and thinks that you must be trying to access an attribute.

Why is the library function call prefixed with node? I would think that the method missing on node is really designed for attribute access so it's interesting that this works.

@jaymzh
Collaborator
@acrmp
Owner

If you are re-opening the node class and defining a new method then foodcritic could look for that and treat it as a built-in method (ignore it). This is probably a good idea.

The node.my_lib_method syntax also works for methods defined in libraries that are not re-opening node - this is what I thought you were using. This seems like a misfeature to me.

The libraries page on the Chef wiki has examples of how you would normally call into code you define in a library:
http://wiki.opscode.com/display/chef/Libraries

@jaymzh
Collaborator

gotcha. Currently this one is biting us - how hard is this feature/fix to implement?

@acrmp
Owner

Something similar is done at present to detect the use of edelights chef-solo-search library:
https://github.com/acrmp/foodcritic/blob/c14fe93bbdf9bb9b3a430f9a4e9f9d5faf968bc7/lib/foodcritic/api.rb#L48

Were you thinking of tackling it yourself? Otherwise I might be able to take a look this weekend assuming you can share the library or enough detail to figure out how to match against it.

Solving this for the general case is a bit harder because there are multiple ways to reopen a class in Ruby.

@acrmp
Owner

Hi Phil. Any update on this?

@jaymzh
Collaborator

Sorry. No, I have no plans to fix this myself, I'm swamped. I was just expressing important to us. In the mean time, we're disabling the check. :(

@acrmp
Owner

Sorry if I was unclear above. I didn't mean you had to implement it (although that would of course be great).

As I said last week, if you can share the library or sufficient detail to get going I should be able to take a look at it.

@jaymzh
Collaborator

Oh, I missed that part. I don't have the code on me nwo, but it's basically:

class Chef
  class Node
    def in_tier?(*tier)
       tier.flatten.include?(node['tier'])
    end
  end
end

It's just a short cut for writing stuff like:

if ['something', 'somethingelse'].include?(node['tier'])
@acrmp
Owner

Thanks. I'll try and take a look this week.

@jaymzh
Collaborator

awesome. lemme know if i can provide anything else.

@acrmp
Owner

Hi Phil,

I've committed a change that should resolve this for you.

Can you test against current master on GitHub and let me know if this is still an issue?

Thanks,

Andrew.

@jaymzh
Collaborator

(ignore the last comment, my branch wasn't uptodate, hence I deleted the comment)

@jaymzh
Collaborator

Confirmed, this looks good.

@jaymzh
Collaborator

How soon can you roll a release?

@jaymzh
Collaborator

Ping?

@acrmp
Owner

Pong. Sorry about the delay.

I'll avoid cutting a release while I've got this head cold so will get onto it this weekend.

Cheers,

Andrew.

@acrmp
Owner

Hi Phil,

This has now been released in 1.3.0.

@jaymzh
Collaborator

Hey. So this does work... but a few suggestions/thoughts:

Currently if you're checking a cookbook that uses a library call defined in a different cookbook it triggers. This isn't necessarily bad, but the error message of accessing attributes inconsistently isn't correct... and is very misleading. If what I did was a method call [node.foo()] then you should instead say something like "unrecognized method"... so people know what's going on.

In fact, my preference here is that F019 would ignore method calls and a new rule would be written that would look for method calls that were invalid...

@acrmp
Owner

Hi Phil,

At present it shouldn't trigger provided the cookbook that contains the library and the cookbook using the library method are under the same cookbook path. Is this not the behaviour you are seeing? Can you provide an example?

Cheers,

Andrew.

@jaymzh
Collaborator

If I call fc on a given cookbook, then no. I need to give it a path with all the right dependencies. Which is fine, but the error message is misleading, so I don't know what I've done wrong. The error is "you didn't give me the cookbook that defines method foo()" but what you say is "inconsistent attribute access."

@acrmp
Owner

Ok. Under the covers these are all method calls:

# i'm accessing an attribute
node.foo

# or I could be calling a method added to Chef::Node
node.foo

# you probably mean this as a method call
# but it could also be used to retrieve an attribute value
node.foo()

If you are explicit when you invoke your method and use parentheses then we could choose to interpret it as a call to a method and treat it differently.

@jaymzh
Collaborator

Yes, that's exactly what I'm suggesting. Currently chef will interpret node.foo as a an attribute access if it doesn't find a method. This is unfortunate and a "feature" even Adam wishes he didn't add.

But what I'd like to do is say that assuming I'm explicit and say node.foo(), it should be exempt from FC019, and then potentially add another rule to validate that method.

@acrmp
Owner

Hi Phil,

The change to ignore explicit method calls with parentheses was shipped in 1.4.0.

Can you test and confirm this one is good to close.

Cheers,

Andrew.

@acrmp
Owner

Closing this one as resolved.

@acrmp acrmp closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.