Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for Rails 7.2 #45

Merged
merged 9 commits into from
Sep 12, 2024
Merged

Add support for Rails 7.2 #45

merged 9 commits into from
Sep 12, 2024

Conversation

svanhesteren
Copy link
Contributor

@svanhesteren svanhesteren commented Sep 5, 2024

Adding support for rails 7.1 by fixing the deprecation of request.show_exceptions?. Now this is controlled by Rails.application.config.action_dispatch.show_exceptions.

NOTE: Wil merge this after the pbbuilder merge, so we can import the gem instead of the github code directly.

@svanhesteren svanhesteren marked this pull request as ready for review September 10, 2024 11:46
@svanhesteren svanhesteren changed the title Update pbbuilder and fix tests. Add support for Rails 7.2 Sep 10, 2024
@svanhesteren svanhesteren requested a review from a team September 11, 2024 12:55
Copy link
Contributor

@skatkov skatkov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My biggest worry is that one of twirp gem versions was breaking our app. If it's still the case, would be great if we will mark this version as not supported.

But generally this looks good to go for me.

Rakefile Show resolved Hide resolved
exception_wrapper = ActionDispatch::ExceptionWrapper.new(backtrace_cleaner, e)
# ExceptionWrapper.show? contains the logic that chooses to pass exceptions through or not based on the
# `Rails.application.config.action_dispatch.show_exceptions` setting of :none, :rescuable and :all
raise e unless exception_wrapper.show?(http_request)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought: nice 👍🏻

rails_twirp.gemspec Outdated Show resolved Hide resolved
.ruby-version Outdated
@@ -1 +1 @@
3.3.0
3.3.3
Copy link
Contributor

@skatkov skatkov Sep 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought: do we really need .ruby-version in a gem's source code? Can we yank this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prob. Most of our codebases have it. But not all. Added multi version testing.

@@ -7,6 +7,7 @@
ActiveRecord::Migrator.migrations_paths = [File.expand_path("../test/dummy/db/migrate", __dir__)]
require "rails/test_help"
require "rails/test_unit/reporter"
require "pry"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought: do we really need pry here for debugging? Would binding.irb be enough?

In pbbuilder, for example, irb doesn't work. But for this gem we can easily debug without pry.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure yanked it out.

in favor of multi version testing
@svanhesteren svanhesteren force-pushed the support-rails-7-2 branch 2 times, most recently from b7fdba4 to 7e03168 Compare September 12, 2024 12:14
@svanhesteren svanhesteren merged commit 372ffe5 into main Sep 12, 2024
4 checks passed
@svanhesteren svanhesteren deleted the support-rails-7-2 branch September 12, 2024 12:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants