AS core_ext refactoring #5996
AS core_ext refactoring #5996
Conversation
|
Need to read the patch carefully for the details, but the overall refactor looks good to me. 👍 over here modulus details. |
| formatter.datetime_format = datetime_format if formatter.respond_to?(:datetime_format=) | ||
| if formatter.respond_to?(:datetime_format=) | ||
| formatter.datetime_format = datetime_format | ||
| end |
There was a problem hiding this comment.
I unwrap one-liners everywhere where they have more (or much more) then 80 symbols. Just like this style)
There was a problem hiding this comment.
Looks good. You could shorten the argument name as well, from datetime_format to just format.
|
Thanks for digging in and polishing these up! Comments inline. |
|
Did you guys test this at all? It fails for me with 5 errors. Mostly dealing with |
|
@larzconwell yep, I found it after discussion. I changed almost all already and I don't have falling tests now. I will push it soon. |
|
Ah okay cool |
|
I've fixed all what we talked about in first commit and add three commits more for discussion. Thanks! |
|
I don't think that we need to change all the string quotes. I'm 👎 for this kind of change because it make harder to dig in the git history. |
|
But maybe it makes sense to change quotes in strings which affected only by refactoring? |
|
Make sense. |
| dup.delete_if { |k, v| h2[k] == v }.merge!(h2.dup.delete_if { |k, v| has_key?(k) }) | ||
| dup. | ||
| delete_if { |k, v| h2[k] == v }. | ||
| merge!(h2.dup.delete_if { |k, v| has_key?(k) }) |
There was a problem hiding this comment.
This is better as one line IMO.
There was a problem hiding this comment.
Too much logic for one line with non-semantic variables IMO :). And line is pretty long. That's why I decided to split this method chain
There was a problem hiding this comment.
👍 I guess the vars are pretty cryptic haha, I think we should rename h2, to something more obvious.
There was a problem hiding this comment.
I was thinking just simply hash or other_hash Anything is better than h2 honestly
|
renamed |
|
I ran the tests and they all pass. There's a lot going on, but most of it is cosmetic and helps to improve the readability of the code-base. |
|
@jeremyf thanks for reporting this. We are helping a lot. |
| @@ -18,7 +18,7 @@ def in?(*args) | |||
| if another_object.respond_to? :include? | |||
| another_object.include? self | |||
| else | |||
| raise ArgumentError.new("The single parameter passed to #in? must respond to #include?") | |||
| raise ArgumentError.new 'The single parameter passed to #in? must respond to #include?' | |||
There was a problem hiding this comment.
Can be shortened to raise ArgumentError, '...'
|
Nice round of additional polish. Thanks @gazay |
Active Support housekeeping and polish
| # "CamelOctopus".pluralize # => "CamelOctopi" | ||
| # "apple".pluralize(1) # => "apple" | ||
| # "apple".pluralize(2) # => "apples" | ||
| # 'post'.pluralize # => "posts" |
There was a problem hiding this comment.
The quote changes are completely unnecessary imo.
I want to make code inside AS core_ext more readable and understandable. Some simple stuff for starters.
Any advice?