turboladen / tailor

A RubyGem that allows for checking standard styling of Ruby files.
146 stars 18 forks source link

Ability to disable checks #92

Closed jaymzh closed 12 years ago

jaymzh commented 12 years ago

The new configuration file format allows specifying different settings for existing checks, but not disabling checks (except for the 'name case' ones). It would be great to be able to enable/disable certain checks both from the config file and optimally from the command line.

turboladen commented 12 years ago

Yeah, you're totally right. At this point, you should be able to set any setting to 'false' to disable it, but that's not apparent at all. I'm planning on spending some more time with the config stuff, now that the design has settled down.

I've thought about allowing CLI params for style, but with all the nesting that I've got going so far, that seems like it could get kinda gnarly. I could flatten out the hierarchy in the config file, I suppose... Do you know of any examples that deal with this well?

jaymzh commented 12 years ago

foodcritic does this, but it's a bit of a different paradigm (it's flat)... Each rule has an implicit rule tag so you can do things like --tags ~FC003

pylint does this incredibly well, actually. It's a great linter...

turboladen commented 12 years ago

I've heard a lot of great things about pylint--I'll check it out.

I'm considering dumping YAML en lieu of just using Ruby for doing the config file--any opinion?

jaymzh commented 12 years ago

I did that for some software and regretted it. YAML isn't really designed for humans to edit. Its very picky about whitespace and formatting. I like ruby since it's a bit more forgiving.

turboladen commented 12 years ago

Yeah, I'm starting to shy away from YAML--seems like it could be problematic for this use case. Thanks for the feedback.

turboladen commented 12 years ago

So I've ditched YAML. Now I'm on the fence as to which route to go with the config file: DSL feel or just a Hash?

Any opinion?

DSL:

Tailor.config do
  formatters ["text"]
  file_set 'lib/**/*.rb' do
    allow_camel_case_methods false
    allow_hard_tabs false
    allow_screaming_snake_case_classes false
    allow_trailing_line_spaces false
    indentation_spaces 2
    max_line_length 80
    max_code_lines_in_class 300 
    max_code_lines_in_method 30
    spaces_after_comma 1
    spaces_before_comma 0
    spaces_before_lbrace 1
    spaces_after_lbrace 1
    spaces_before_rbrace 1
    spaces_in_empty_braces 0
    spaces_after_lbracket 0
    spaces_before_rbracket 0
    spaces_after_lparen 0
    spaces_before_rparen 0
    trailing_newlines 1
  end 
end

Hash:

Tailor.config do
  {
    file_sets: [
      file_list: 'lib/**/*.rb,
      style: {
        allow_camel_case_methods: false,
        allow_hard_tabs: false,
        allow_screaming_snake_case_classes: false,
        allow_trailing_line_spaces: false,
        indentation_spaces: 2,
        max_code_lines_in_class: 300,
        max_code_lines_in_method: 30,
        max_line_length: 80,
        spaces_after_comma: 1,
        spaces_before_comma: 0,
        spaces_before_lbrace: 1,
        spaces_after_lbrace: 1,
        spaces_before_rbrace: 1,
        spaces_in_empty_braces: 0,
        spaces_after_lbracket: 0,
        spaces_before_rbracket: 0,
        spaces_after_lparen: 0,
        spaces_before_rparen: 0,
        trailing_newlines: 1
      }
    ],
    formatters: ['text']
  }
end

Personally, I like the DSL feel better, but I realize that you don't get as much control (which is part of the point in switching to Ruby for config). Who knows if anyone will even want/need to get that crazy with defining their config, but I'd rather make the decision now.

Also, FWIW, the concept of file sets is new... I want to be able to specify different styles for different file sets (i.e. I might like to allow longer lines in my Rails app files, but not lib files; or skip checking spacing around { and } in RSpec). So in case you're wondering, that's what that means in those data structures above. :)

Thoughts?

jaymzh commented 12 years ago

I always prefer a real language... maintaining (or learning) another DSL is a pain.

turboladen commented 12 years ago

You're right on the DSL thing; when they're needed, they make sense--in this case, it's not needed. I've settled on the following format:

Tailor.config do |config|
  config.formatters "text"
  config.file_set 'lib/**/*.rb' do |style|
    style.allow_camel_case_methods = false
    style.allow_hard_tabs = false
    style.allow_screaming_snake_case_classes = false
    style.allow_trailing_line_spaces = false
    style.indentation_spaces = 2
    style.max_code_lines_in_class = 300
    style.max_code_lines_in_method = 30
    style.max_line_length = 80
    style.spaces_after_comma = 1
    style.spaces_before_comma = 0
    style.spaces_before_lbrace = 1
    style.spaces_after_lbrace = 1
    style.spaces_before_rbrace = 1
    style.spaces_in_empty_braces = 0
    style.spaces_after_lbracket = 0
    style.spaces_before_rbracket = 0
    style.spaces_after_lparen = 0
    style.spaces_before_rparen = 0
    style.trailing_newlines = 1
  end
end

Again, rulers that take a number can also take false to turn them off. All of these are also configurable as CLI options.

Closing from changes in fc059206102ac5328933221109c767c868b5c469.

jaymzh commented 12 years ago

Perfect. Is this in 1.0.0?

jaymzh commented 12 years ago

Also, is there a setting for indentation of line continuation?

I.e.

if someconditional_that == is_really_long.function().stuff() or
    another_condition == some_thing
  print "boop\n"
end

I don't see a setting for line that line after the 'or' - does that feature exist, or should I make a bug?

turboladen commented 12 years ago

This is in 1.0.0, although with slight variation from what I posted. Do a tailor --create-config and you should get a template at .tailor to start with. I checked in my .tailor with this project, so you could take a look at that for an example if the README doesn't quite cut it. I can totally see how this is one of those things that might make sense to me because I wrote it, but might need more explaining than I've done; sorry if that's the case--let me know and I'll try to improve.

In 1.0.0, tailor doesn't allow for varying line continuation--in the case above, it'd expect:

if someconditional_that == is_really_long.function().stuff() or
  another_condition == some_thing
  print "boop\n"
end

...where line 2 is expected to be indented 2 spaces due to the or, then line 3 is expected to be indented 2 spaces because it's the first statement inside the if.

If you want to be able to specify something different than that, then yeah, go ahead and bug that and I'll try to lump that in with the release for allowing varying indentation styles (I'm thinking 1.1.0 right now, but we'll see).