Integrated RuboCop as part of the build.

solnic / virtus

[DISCONTINUED ] Attributes on Steroids for Plain Old Ruby Objects

MIT License

3.77k stars 228 forks source link

Integrated RuboCop as part of the build. #349

Open dblock opened 8 years ago

dblock commented 8 years ago

Something discussed in https://github.com/solnic/virtus/pull/343.

For a contributing developer the build would fail on something like what is being added as part of feedback in #343. This also removes any conversation about style, Rubocop just enforces those automagically. I first ran rubocop -a, then rubocop --auto-gen-config and reviewed some infrequent violations and fixed them by hand.

dblock commented 8 years ago

Btw, @envygeeks @Bartuz I didn't mean to start too many debates :) I just wanted the person who made a PR, #343 to catch the style issue they were asked to fix before someone had to look at the code and waste time. So please tell me what you'd like me to change in this PR for it to be merged or whether you'd rather not, I am happy to help!

envygeeks commented 8 years ago

For me, the keyword style hashes has to go, it promotes inconsistency and I am very consistent with my code in that aspect in that I always explicitly do { :key1 => :val1, "key2" => "val2" } for absolute consistency in all my hashes, def(keyword: val) so there is a very clear distinction in my code as to what it's doing. Ruby introduced code inconsistency in 1.9 with that and it's been an emacs vs. vim war ever since, I never intervene unless one is thrust upon me, meaning if you wish to do { key: val } in your code when submitted, I'll never ask it to change, as I don't ask you to ask me to change.

And the fail part.

dblock commented 8 years ago

I've merged https://github.com/dblock/virtus/pull/1 from @Bartuz, so the or in the control flow is no longer flagged.

dblock commented 8 years ago

@envygeeks I undid a few things in this PR:

Hash rockets are enforced. Most code here was hash rockets, so be it. I think we can all agree that consistency is better than inconsistency, so some places were auto-corrected from Ruby 1.9 syntax to hash rockets.
Raise is back instead of fail everywhere.
Or in control flow is allowed, thx @Bartuz.
Hash values in a table layout must align.

I care more about having an automatic style in Virtus than what the style is, so LMK what else needs to go for this to be merged.

envygeeks commented 8 years ago

Yay, :heart: thanks for the changes. I'm happy with it if @booch , I don't know if he's had time to review.

dblock commented 8 years ago

Bump @booch ?

booch commented 8 years ago

Sorry, I thought I had commented on this. Not sure if I never hit Enter, or if it got eaten.

I really like RuboCop, but I'd really prefer that we use it a bit less aggressively. For example, some of us (myself, and likely @solnic) prefer to keep extra blank lines to aid readability. And I'd prefer RSpec let to consistently use {} instead of do/end, instead of the 1-line versus multi-line rule.

I'm gonna sit on this for just a bit. I might just rework it to be a bit less aggressive, then accept it. I would prefer to keep RuboCop and most of these settings.

envygeeks commented 8 years ago

What is the status of this?

dblock commented 8 years ago

I think it's up to @booch but I am happy to help with whatever!

envygeeks commented 8 years ago

I'm :+1: on this, we can adjust whatever we need to later. No reason to block this pull. /cc @booch