Support Rails 6.1.6.1 #1104
Support Rails 6.1.6.1 #1104
Conversation
stevepolitodesign
force-pushed
the
support_rails_6_1_0
branch
2 times, most recently
from
c86de29 to
710b7d7
Compare
stevepolitodesign
force-pushed
the
support_rails_6_1_0
branch
2 times, most recently
from
2c9a326 to
7f0329b
Compare
|
@mike-burns that works for me! I ended up adding the method to the I ended up borrowing a lot from the existing method, but let me know if this is overkill. |
| append_file "Gemfile", %(\ngem "sidekiq"\n) | ||
| gem "sidekiq" |
There was a problem hiding this comment.
That was my fault. I was accidentally on main when I pushed that up 🤦.
Is it worth removing that commit before I merge this in?
Also, I wonder if we should enable a branch protection rule to prevent this from happening again?
There was a problem hiding this comment.
I think branch protection would be great! As for the commit, it's small and harmless, so I think we can leave it as-is. This PR makes a change to the same line, and also improves it, so that's good.
| str = [] | ||
| str << indentation | ||
| str << "gem #{parts.join(", ")}" | ||
| append_file "Gemfile", %(\n#{str.join}\n), verbose: false |
There was a problem hiding this comment.
The original Rails method uses append_file_with_newline. Is that method not reversible while append_file is?
There was a problem hiding this comment.
append_file_with_newline calls gsub_file which unfortunately is not reversible.
lib/suspenders/actions.rb
Outdated
| versions.each do |version| | ||
| parts << quote(version) | ||
| end | ||
| message << " (#{versions.join(", ")})" |
There was a problem hiding this comment.
It looks like message is not being used. In the original method, it is used for logging on this line:
log :gemfile, message
Do you think logging would be valuable? Otherwise, we can delete it.
There was a problem hiding this comment.
Good catch. Updated here: d55520a
| parts << quote(options) unless options.empty? | ||
|
|
||
| str = [] | ||
| str << indentation |
There was a problem hiding this comment.
Not a big deal, but I prefer how it reads in the original method:
str = "gem #{parts.join(", ")}"
str = indentation + strAlso, the original method uses in_root, but I suppose that is not necessary here.
There was a problem hiding this comment.
Oddly enough, I was getting failures when I used that implementation, but I'm not totally sure why 🤔
I think this might have also been the result of having run standardrb --fix.
| @@ -1,4 +1,5 @@ | |||
| require "active_job/logging" | |||
| require "active_job/log_subscriber" | |||
There was a problem hiding this comment.
Is this also necessary for Rails 6.1?
There was a problem hiding this comment.
Oddly enough, yes. Without it, we get uninitialized constant ActiveJob::Logging::LogSubscriber.
This commit does the bare minimum to support Rails 6.1.6.1 Because the `gem` command is no longer reversible, we decided to override the method by borrowing as much from the [existing method](https://github.com/rails/rails/blob/04972d9b9ef60796dc8f0917817b5392d61fcf09/railties/lib/rails/generators/actions.rb#L22). The main difference being that we call `append_file` instead of `gsub_file`, so that we can preserve reversibility. However, #1066 provides a more elegant solution. Closes #1060
stevepolitodesign
force-pushed
the
support_rails_6_1_0
branch
from
d55520a to
6409e1b
Compare
This commit does the bare minimum to support Rails 6.1.6.1
Because the
gemcommand is no longer reversible, we decided to override the method by borrowing as much from the existing method. The main difference being that we callappend_fileinstead ofgsub_file, so that we can preserve reversibility. However, #1066 provides a more elegant solution.Closes #1060