rules in code?

[kcbraunschweig]

I guess this is more of a feature request, but does it make sense for the rules to be in the code of the app rather than a configuration? It seems wrong to have to rebuild the application to add new rules.

[acrmp]

Hi KC,

I think from your description this has more to do with how foodcritic requires its own libraries. I've tweaked this - can you pull from master and you should see changes reflected without a build:
$ bin/foodcritic my_cookbook_path

Cheers,

Andrew.

[acrmp]

Or were you saying you'd like an option to define site-specific cookbook rules in config external to the gem?

[kcbraunschweig]

Yes, that's what I meant. I suspect the defaults that come with the gem should continue to reflect what the community thinks should be the standards. Rather than making people fork and rebuild the gem to add their own organizational standards which may not be suitable for the larger community, which they then have to maintain, why not just allow a config file/dir where you can add in custom rulesets independent of the gem? And maybe disable defaults as well so you don't have to pass it on the command line.
I know, i know, I shoulda taken a stab at this myself before bugging anyone, but I kinda wondered if people thought this approach made sense.

[acrmp]

Hi KC,

Are you able to give specific examples of rules that you think people may want to apply that it would not make sense to contribute back?

I'd think I prefer instead to encourage contributions back and have a set of 'default' rules that are normally run. Users can then choose to apply more specialised rules (outside the default set but included with the gem) if it is appropriate for their situation.

Assuming other users may share your itch then this should prevent duplication of effort (everyone gets to benefit from your custom rule) and ensures that changes in the API that cause breakage will be picked up.

Does this work for you?

[kcbraunschweig]

I expect some companies will want things that no one else would but I'll close this until I have specific examples.

[jaymzh]

I have plenty of examples.

I may want people to not use search (for scalability reasons) in cookbooks.
Or I may want attributes to all start with a top-level container of "companyname" (default[:company][:cookbook][:foo])
Or I may want to ensure that all cookbooks that touch some magical default["company"]["_magical"] attributes make sure to also touch some other magical thing.
Or that all copyrights are [email protected]
Or that...

there's a BILLION reasons this is a good idea and I was going to request the same thing until I saw it was already here.

[kcbraunschweig]

Re-opening this issue at jaymzh's request.

[jaymzh]

As a side-note, the way I'd sorta see this being implemented is a config entry that defines a site-specific library directory and a list of rules to load (the same way there's a list of built-in ones in the code). At run-time you'd walk that list and load them the same way (I don't know crap about cucumber, so I don't know how easy/hard it is to do that).

[acrmp]

Ok, I'm convinced it would be useful :-)

Versioning
The biggest issue for me in providing this is keeping to the versioning contract. Foodcritic tries to adhere to the Rubygems Rational Versioning policy but currently this only applies to the command line interface - I don't make any assurances about the API available to the rules.

I've had specs for the rule API on my todo list for a while, so this is extra impetus to get this done so I won't break your stuff inadvertently on minor releases. I can't responsibly ship this feature without these specs being in place.

Implementation
I'd keep the implementation simple to start with and add a -I (--include) option that accepts a rule file path which is loaded after the built-in rules. An error in loading the file would die. The rule loading isn't really connected to Cucumber and it's dead easy to extend that to walking a directory tree if it makes sense.

Cheers,

Andrew.

[jaymzh]

The implementation sounds good. Would you load every rule in that directory? What if there was a README, would you croak?

For versioning... I don't know enough about ruby (yet). Ideally there'd be something like so-versions that we have in c libraries... or perhaps that way mozilla does plugin versioning where the rules say what version they believe they work on?

[acrmp]

The Ruby way is just to randomly break shit test everything.

If I make a Category 3 change to the rules API which I expect to break your existing code then I would bump the Foodcritic major version. Normally you would express compatibility through gem constraints - you might choose the wonderfully named sperm operator:
http://docs.rubygems.org/read/chapter/16#page74

This assumes a slightly different model then what we've said above, where you would create a Gemfile in your rules directory that referenced foodcritic and run bundle exec foodcritic. This is a bit more heavyweight and cumbersome to get started than the approach we were outlining above but less likely to break.

Need to give it some more thought.

[jaymzh]

Yeah building gem files is pretty heavy-weight.

I'm fine with saying API changes mean a major version bump.

Also, <3 the sperm operator.

[acrmp]

Hi guys,

This feature is in 1.0.0 - please give it a try and let me know how you get on. It still needs API documentation but you should be able to get started.

This existing section of the docs relates more to modifying the existing codebase but may still be helpful:
http://acrmp.github.com/foodcritic/#writing-a-new-rule

# my_custom_rule.rb
rule "MYORG001", "My custom rule" do
  tags %w{style attributes}
  recipe do |ast|
    attribute_access(ast, :type => :string).map{|ar| match(ar)}
  end
end

$ foodcritic -I my_custom_rule.rb cookbook

[jaymzh]

Andrew, Thanks for this!

As you mentioned, there's not really docs here, so I'm going to ask a few questions.

First, I couldn't get pry to dump the API:

[1] pry(#<FoodCritic::RuleDsl>)> Api.instance_methods.sort
NameError: uninitialized constant FoodCritic::RuleDsl::Api
from (pry):1:in `block in load'

Second, I tried to use the example above to match all gem_package with like:

recipe do |ast|
  find_resources(ast, 'gem_package').map{|ar| match(ar)}
end

But it crashes:

/home/phild/.rvm/gems/ruby-1.9.3-p0/gems/foodcritic-1.0.0/lib/foodcritic/api.rb:122:in `merge!': can't convert String into Hash (TypeError)
    from /home/phild/.rvm/gems/ruby-1.9.3-p0/gems/foodcritic-1.0.0/lib/foodcritic/api.rb:122:in `find_resources'
    from ./fc_fb001.rb:4:in `block (3 levels) in load'

[acrmp]

Hi Phil,

You want to be in the context of a rule, so declare one first. When you type exit in the commands below you will be bounced to the next Pry binding, inside the rule you just defined.

$ foodcritic -r cookbooks/my_cookbook
pry(#<FoodCritic::RuleDsl>)> rule "MYRULE123", "My new rule" do
  recipe do |ast|
    binding.pry
  end
end
pry(#<FoodCritic::RuleDsl>)> exit

Now you can list the available methods and ask Pry to show you the documentation.

The rule will be invoked for each recipe file - use exit to jump to the next invocation.

pry(#<FoodCritic::RuleDsl>)> Api.instance_methods.sort
pry(#<FoodCritic::RuleDsl>)> show-doc find_resources
pry(#<FoodCritic::RuleDsl>)> find_resources(ast, {:type => :gem_package}).map{|pkg| match(pkg)}

Hope this helps.

Cheers,

Andrew.

[jaymzh]

Oh, that's awesome, thanks! Is it possible to specify or-able requirements? I tried:

{:type => %w{gem_package chef_gem}
{:type => :gem_package, :type => :chef_gem}

but neither worked. I can obviously make two rules... but it seemed like a useful thing to do.

(chef_gem is coming in 0.10.10)

[acrmp]

You can't pass an array of types to find_resources at the moment (though that's a good idea).

You can achieve the same effect by calling find_resources with no type specified and then filtering on the result:

find_resources(ast).find_all do |res|
  %w{gem_package chef_gem}.include?(resource_type(res))
end.map{|pkg| match(pkg)}

[jaymzh]

Awesome. Foodcritic is seriously going save me so much pain and effort. Thanks so much for knocking out so many RFEs so quickly!

[jaymzh]

BTW, I'm now using this in production. It is awesome.

[acrmp]

Thanks! I'm expecting to make some progress on the documentation this week.