Integrated RuboCop as part of the build. #349
Conversation
|
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! |
|
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 And the fail part. |
| @@ -84,7 +82,7 @@ def determine_type_from_primitive(primitive) | |||
| descendants.select(&:primitive).reverse_each do |descendant| | |||
| descendant_primitive = descendant.primitive | |||
| next unless primitive <= descendant_primitive | |||
| type = descendant if type.nil? or type.primitive > descendant_primitive | |||
| type = descendant if type.nil? || type.primitive > descendant_primitive | |||
There was a problem hiding this comment.
What do you mean? It is correct now, isn't it? || and && should be only used in conditions (like above)
There was a problem hiding this comment.
@dblock no, or is the same as ||. Ruby programmers just choose to use or only for flow control, but at the end of the day it doesn't matter, he was choosing to be more verbose.
There was a problem hiding this comment.
Nevermind, in this case it doesn't matter indeed.
There was a problem hiding this comment.
@envygeeks That's incorrect, || has a higher precedence than or.
a = false or true sets a to false, and a = false || true sets a to true, since = precedence is lower than || but higher than or. SO
There was a problem hiding this comment.
Yes, I know that, but that is irrelevant in this case so I didn't feel the need to even add that bit.
|
@envygeeks I undid a few things in this PR:
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. |
|
Yay, ❤️ thanks for the changes. I'm happy with it if @booch , I don't know if he's had time to review. |
|
Bump @booch ? |
|
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 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. |
| @@ -3,7 +3,16 @@ | |||
| describe Virtus::Attribute, '#set_default_value' do | |||
| let(:object) { described_class.build(String, options.merge(:name => name, :default => default)) } | |||
|
|
|||
| let(:model) { Class.new { def name; 'model'; end; attr_reader :test } } | |||
There was a problem hiding this comment.
I find this original one-liner a lot more readable than the 9-line replacement. Ugh.
There was a problem hiding this comment.
Personally I find it harder. Also very difficult to see diffs in 1-liners as code changes. Accent on "personally".
There was a problem hiding this comment.
I agree, that onel iner is pretty hard to read.
|
What is the status of this? |
|
I think it's up to @booch but I am happy to help with whatever! |
|
I'm 👍 on this, we can adjust whatever we need to later. No reason to block this pull. |
Something discussed in #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, thenrubocop --auto-gen-configand reviewed some infrequent violations and fixed them by hand.