Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

rules in code? #8

Closed
kcbraunschweig opened this Issue · 20 comments

3 participants

@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
Owner

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
Owner

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
Owner

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
Collaborator

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 team@mycompany.com
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
Collaborator

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
Owner

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
Collaborator

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
Owner

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
Collaborator

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 acrmp referenced this issue from a commit
@acrmp Extend API specs, refs #8. 40f9db9
@acrmp
Owner

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
@acrmp acrmp closed this
@jaymzh
Collaborator

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
Owner

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
Collaborator

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
Owner

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
Collaborator

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

@jaymzh
Collaborator

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

@acrmp
Owner

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

@acrmp acrmp referenced this issue from a commit
@acrmp Website refresh, refs #8.
- Moved to Jekyll and Bootstrap 2
- API docs added
e849e70
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.