remove activesupport dependency by creating package_manager_version#6667
remove activesupport dependency by creating package_manager_version#6667jakecoffman merged 2 commits intomainfrom
Conversation
|
Looks like this is mostly working now, just some Active Support extensions we could either monkey-patch back in or rewrite with plain-ole-Ruby. The dry-run HTTP request output is nice so we'll definitely want to add that back in somehow. I will pause now for any input. Is this a good idea? |
|
ActiveSupport is helpful is you use a significant part of the functionality it provides. In our case, we seem to be using just minimal functionality but we get the disadvantages of globally monkeypatching a lot of core classes. If we can do this, I'm all for it. |
|
I think I got everything. I opted to replace ActiveSupport extensions instead of monkey patching them back in. Also made the executive decision to delete bin_run_spec.rb since we have a suite of tests that use bin/run with more thorough assertions. |
deivid-rodriguez
left a comment
There was a problem hiding this comment.
Looks great to me, awesome job!
I guess this could be logically divided into two commits, one to remove the end to end fetcher specs, the other one to remove the activesupport dependency? Happy to rebase and squash this to do that, just to get git history a bit cleaner.
058a357 to
63f1249
Compare
|
@deivid-rodriguez rebased |
deivid-rodriguez
left a comment
There was a problem hiding this comment.
Really happy about this, thanks for the nice work!
Sparked by my troubles in #6658 to get the Updater test working, and remembering that updating activesupport to the latest version broke things in #6522, I asked why we need it at all?
It seems more straight forward to elevate a method (
package_manager_version) to be implemented in the base, and have file_fetchers implement it than to use Notifications. That and we get to delete a dependency that is giving us trouble!