sgruhier / foundation_rails_helper

Rails Helper for Zurb Fondation framework
MIT License
153 stars 83 forks source link

Add style linting with rubocop #143

Closed dgmstuart closed 8 years ago

dgmstuart commented 8 years ago

replaces #119

dgmstuart commented 8 years ago

@dsandstrom I don't think it's possible to enforce single quotes in app code and double quotes in spec code. I've enforced the former and excluded specs from it.

Please review and merge.

dgmstuart commented 8 years ago

Hmm. The failing build is because Rubocop only supports Ruby 2.0 and above.

Whatever way you cut it, adding Rubocop is going to be a breaking change: we'll need to release a v2.0 of the gem that:

The question is whether we roll foundation 6 into that release as well, or separate it out into a 3.0 release.

dsandstrom commented 8 years ago

For double quotes in specs, I use something like this in my spec directory:

inherit_from:
  - ../.rubocop.yml

StringLiterals:
  EnforcedStyle: double_quotes
  Exclude:
    - './rails_helper.rb'
    - './spec_helper.rb'
dsandstrom commented 8 years ago

I agree on dropping 1.9.3 support. It should be compatible with that Ruby version, but there's no use in spending time on maintaining support.

For versioning, the Rails 5 update would be 1.3.0 like you suggested somewhere else. I don't know of any breaking changes. The Foundation 6 update should be 2.0.0.

dgmstuart commented 8 years ago

@dsandstrom I think I tried your spec directory approach and it didn't seem to work - or at least, it didn't pick up and use the nested spec/rubocop.yml when running rubocop from the top level.

Do you run rubocop twice? Once for the app code and the other for the spec code? I'd worry that the latter is less likely to get routinely run in development, which would be a bit of a pain.

dgmstuart commented 8 years ago

@dsandstrom cool - so if you're saying you don't want to release a 2.0.0 without Foundation 6 then this PR needs to be on the same branch as that code - not based off master.

What do you reckon? Should I create a new PR based off the foundation-6 branch?

dsandstrom commented 8 years ago

Should I create a new PR based off the foundation-6 branch?

Yes that is what I was going to suggest.

dgmstuart commented 8 years ago
dsandstrom commented 8 years ago

I think we should hold off on merging this PR. We should make a foundation-5 maintenance branch before merging foundation-6. Then, we can merge this PR into foundation-5.

dgmstuart commented 8 years ago

I see where you're going, but how about if we were to keep one major version for foundation-5 and another for foundation-6?

If we're going to do that, then we should merge this to master, release 2.0 (if you agree that dropping support for 1.9.3 is a breaking change), then have 2.x be for foundation 5 and 3.x be for Foundation 6.

dsandstrom commented 8 years ago

Sounds reasonable.

dgmstuart commented 8 years ago

Thanks for merging. I find it useful to delete branches once they're merged - what do you think?

dsandstrom commented 8 years ago

Yes, we should also delete ajax and without_form_for, they're both abandoned.

dsandstrom commented 8 years ago

Done