Move Array#to_sentence monkey-patch from ActiveSupport to extend/ #14847
Conversation
|
Review period will end on 2023-03-02 at 01:42:11 UTC. |
BrewTestBot
added
the
waiting for feedback
| @@ -3,6 +3,7 @@ | |||
|
|
|||
| require "ast_constants" | |||
| require "rubocops/extend/formula_cop" | |||
| require "utils/array" | |||
There was a problem hiding this comment.
Note I put the util in a separate file, despite being at the top Utils level, so that this rubocop could import just the method it needs, and not the bulk of the Utils namespace. As a bonus, rubocop no longer has a direct active_support require, but still makes use of active_support methods such as present?
There was a problem hiding this comment.
@dduugg IMO it's worth either just importing all of Utils in rubocop or moving this to another namespace, I think defining modules over multiple files is a confusing behaviour for non-Rubyists (as you don't see the same in most other ecosystems).
| @@ -17,7 +17,8 @@ | |||
| require "active_support/core_ext/object/try" | |||
| require "active_support/core_ext/array/access" | |||
| require "active_support/core_ext/string/inflections" | |||
| require "active_support/core_ext/array/conversions" | |||
| require "active_support/core_ext/kernel/reporting" | |||
| require "active_support/core_ext/hash/keys" | |||
There was a problem hiding this comment.
These are transitive requires that we still need, for now.
dduugg
force-pushed
the
no-to_sentence
branch
3 times, most recently
from
01b3090 to
ca62249
Compare
There was a problem hiding this comment.
Nice work so far!
I reckon we can eventually do similarly for blank?/present?/presence and they should remain as actual monkey patches rather than new Utils methods.
I actually have a mild preference about this remaining a monkey patch (outside of ActiveSupport) but I feel much less strongly than for this.
| @@ -3,6 +3,7 @@ | |||
|
|
|||
| require "ast_constants" | |||
| require "rubocops/extend/formula_cop" | |||
| require "utils/array" | |||
There was a problem hiding this comment.
@dduugg IMO it's worth either just importing all of Utils in rubocop or moving this to another namespace, I think defining modules over multiple files is a confusing behaviour for non-Rubyists (as you don't see the same in most other ecosystems).
BrewTestBot
removed
the
waiting for feedback
|
Review period ended. |
dduugg
force-pushed
the
no-to_sentence
branch
3 times, most recently
from
5533188 to
e4e91da
Compare
|
@MikeMcQuaid PR updated to keep |
dduugg
changed the title
brew stylewith your changes locally?brew typecheckwith your changes locally?brew testswith your changes locally?I'm unable to remove the String inflections ActiveSupport dependency after merging #14778 because it's a transitive require of Array conversions. Thus, in this PR, I've introduced a Utils method for the only conversion we appear to use,
to_sentence.