Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Thoughts on more style-y lint checks #15

Closed
jaymzh opened this Issue · 16 comments

3 participants

@jaymzh
Collaborator

I'd like to start doing checks that are common to more typical lint tools... i.e. styleguide-like changes... for example

if(foo)

should be caught and told to make it:

if foo

Currently, since we're using an AST, I think all of that data is lost. I forsee either a separate tool.... or perhaps two different passes in foodcritic.

This could also relate to the excepting-rules-inline RFE (#10).

@acrmp
Owner

Thanks for this. I'm interested to know what rules you think would be useful. In this instance the parentheses would come through so you could make use of them.

I'm particularly interested in what additional checks we can implement to catch cookbook errors earlier. I'm less interested in general-purpose Ruby linting because other projects are aiming to tackle that. For example Laser:
https://github.com/michaeledgar/laser

In an ideal world foodcritic can focus on Chef DSL specific linting, but leaning on general-purpose Ruby lint tooling to flag those issues as well.

@jaymzh
Collaborator

There were two errors in that example - the lack of space and the paryn

Also:

  • trailing white space
  • foo==bar instead of foo == bar
  • foo="bar" instead of foo = "bar"
  • 4-space indents instead of 2
  • tabs instead of spaces

... and so on.

Can Laser work on cookbooks with the Chef DSL? Won't it choke?

@acrmp
Owner

I've not tried applying Laser to chef cookbooks, but it's all just Ruby so in theory it should work fine.

If you give a try please report back with how you get on.

@jaymzh
Collaborator

wellll... it's... not JUST ruby. You can't stick it through a ruby interpreter alone, you need a bunch of setup to enable the Chef DSL. IT doesn't know what "file" or "template" or "cookbook_file" is.

I have not had much luck:

/home/phild/.rvm/gems/ruby-1.9.3-p0/gems/laser-0.7.0.pre2/lib/laser/analysis/control_flow/cfg_builder.rb:182:in `build_formal_args': undefined method `is_block?' for nil:NilClass (NoMethodError)
    from /home/phild/.rvm/gems/ruby-1.9.3-p0/gems/laser-0.7.0.pre2/lib/laser/analysis/control_flow/cfg_builder.rb:904:in `block (3 levels) in walk_block_body'
    from /home/phild/.rvm/gems/ruby-1.9.3-p0/gems/laser-0.7.0.pre2/lib/laser/analysis/control_flow/cfg_builder.rb:80:in `with_self'
    from /home/phild/.rvm/gems/ruby-1.9.3-p0/gems/laser-0.7.0.pre2/lib/laser/analysis/control_flow/cfg_builder.rb:903:in `block (2 levels) in walk_block_body'
....
@acrmp
Owner

You don't have to eval the code to lint it. A general-purpose Ruby lint tool should just see a method being passed a block.

There is an existing Laser issue here that matches your stack trace:
michaeledgar/laser#13

@jaymzh
Collaborator

That's a very good point. I only had limited time today but I'll continue to play with Laser and also there's some others like Dust.

@jaymzh
Collaborator

So much for Dust/Nitpick: kevinclark/dust#2

@jaymzh
Collaborator

It seems like any recipe triggers that Laser bug... even just a simpler user definition.

@jaymzh
Collaborator

reek doesn't test for typical style stuff
checkr doesn't even build in modern ruby.

And I can't find anything else that looks even remotely promising. Am I missing any?

@jaymzh
Collaborator

Pelusa looks like a good framework. I'd have to add the checks I care about... but I'm trying to work around a bug that prevents me from... running it:
codegram/pelusa#18

@jaymzh
Collaborator

The Pelusa issue I pointed two was apparently just one bug that prevented it from processing recipes. I've filed:
codegram/pelusa#23

In the meantime a new version came out which fixes Issue 18, but it's still incompatible with recipes, afaict.

@jaymzh
Collaborator

Pelusa now doesn't crash on recipes... but it ONLY handles class-level checks, which is essentially useless for cookbooks. I've yet to find anything out there that does anything remotely close to what I want.

Given that, are you still opposed to adding more ruby-lint-style things into FC?

@acrmp
Owner

Still holding off. Have you seen tailor and cane? @turboladen has been making some serious improvements to tailor recently.

https://github.com/turboladen/tailor
https://github.com/square/cane

@turboladen

This certainly seems up tailor's alley, but due to the recent redesign I probably wouldn't look to add it in there for a bit. Please feel free to log an issue though--I'd love to take a look at it when a lot of the code settles down.

@acrmp
Owner

Awesome. Thanks Steve.

@acrmp
Owner

I'm closing this one as people seem to be using tailor with some success for checking style.

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