Skip to content

Fix the type of handlers options passed to render #2521

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jan 20, 2022
+9 −9
Merged

Fix the type of handlers options passed to render #2521

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7e6ab09
Fix the type of handlers options passed to render
alpaca-tc Sep 30, 2021
fbdaa64
Passing formats or handlers to render :template and friends is deprec…
alpaca-tc Oct 3, 2021
96a08e0
Fix the type of options passed to render
alpaca-tc Oct 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions features/view_specs/view_spec.feature
View file
Original file line number Diff line number Diff line change
@@ -80,7 +80,7 @@ Feature: view spec
it "displays the widget" do
assign(:widget, Widget.create!(:name => "slicer"))
render :template => "widgets/widget.html.erb"
render :template => "widgets/widget"
expect(rendered).to match /slicer/
end
@@ -103,7 +103,7 @@ Feature: view spec
it "displays the widget" do
assign(:widget, Widget.create!(:name => "slicer"))
render :template => "widgets/widget.html.erb", :layout => "layouts/inventory"
render :template => "widgets/widget", :layout => "layouts/inventory"
expect(rendered).to match /slicer/
end
@@ -162,7 +162,7 @@ Feature: view spec
it "displays the widget" do
widget = Widget.create!(:name => "slicer")
render :partial => "widgets/widget.html.erb", :locals => {:widget => widget}
render :partial => "widgets/widget", :locals => {:widget => widget}
expect(rendered).to match /slicer/
end
6 changes: 3 additions & 3 deletions lib/rspec/rails/example/view_example_group.rb
View file
Original file line number Diff line number Diff line change
@@ -150,10 +150,10 @@ def _default_render_options
match = path_regex.match(_default_file_to_render)

render_options = {template: match[:template]}
render_options[:handlers] = [match[:handler]] if match[:handler]
render_options[:handlers] = [match[:handler].to_sym] if match[:handler]
render_options[:formats] = [match[:format].to_sym] if match[:format]
render_options[:locales] = [match[:locale]] if match[:locale]
render_options[:variants] = [match[:variant]] if match[:variant]
render_options[:locales] = [match[:locale].to_sym] if match[:locale]
render_options[:variants] = [match[:variant].to_sym] if match[:variant]

render_options
end
6 changes: 3 additions & 3 deletions spec/rspec/rails/example/view_example_group_spec.rb
View file
Original file line number Diff line number Diff line change
@@ -151,19 +151,19 @@ def _default_file_to_render; end # Stub method
it "converts the filename components into render options" do
allow(view_spec).to receive(:_default_file_to_render) { "widgets/new.en.html.erb" }
view_spec.render
expect(view_spec.received.first).to eq([{template: "widgets/new", locales: ['en'], formats: [:html], handlers: ['erb']}, {}, nil])
expect(view_spec.received.first).to eq([{template: "widgets/new", locales: [:en], formats: [:html], handlers: [:erb]}, {}, nil])
end

it "converts the filename with variant into render options" do
allow(view_spec).to receive(:_default_file_to_render) { "widgets/new.en.html+fancy.erb" }
view_spec.render
expect(view_spec.received.first).to eq([{template: "widgets/new", locales: ['en'], formats: [:html], handlers: ['erb'], variants: ['fancy']}, {}, nil])
expect(view_spec.received.first).to eq([{template: "widgets/new", locales: [:en], formats: [:html], handlers: [:erb], variants: [:fancy]}, {}, nil])
end

it "converts the filename without format into render options" do
allow(view_spec).to receive(:_default_file_to_render) { "widgets/new.en.erb" }
view_spec.render
expect(view_spec.received.first).to eq([{template: "widgets/new", locales: ['en'], handlers: ['erb']}, {}, nil])
expect(view_spec.received.first).to eq([{template: "widgets/new", locales: [:en], handlers: [:erb]}, {}, nil])
Copy link
Member

Choose a reason for hiding this comment

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

I would feel confident with merging this to main if we had a confirmation that with to_sym all supported Rails versions (from soft-supported 5.0 and 5.1) up to 6.1 still work fine.

Can you recall, what was the failing cucumber scenario for 7.0 without .to_sym exactly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't recall but I think that calling render without arguments raises ActionView::MissingTemplate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems that an exception is raised if the path described in the description contains handler, locales, etc. as shown below. Sorry I don't know that there is such a test in rspec-rails' cucumber.

RSpec.describe "root/index.html.erb", type: :view do # also xxx.html.slim, xxx.en.html.erb 
  it { render } #=> raise ActionView::MissingTemplate
end

Copy link
Member

Choose a reason for hiding this comment

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

My concern is that the change would break on an older version of Rails. As this is something that was not thought forward to be changed in the future, there might not be a test for it. Sorry for being extra cautious.

Copy link

Choose a reason for hiding this comment

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

@pirj In that case would it be possible to release this as a new major version

Copy link
Contributor

Choose a reason for hiding this comment

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

@pirj So I just saw this discussion after making a similar commit to the PR I opened a few days ago. I'm a little confused as to what the concern is.

We know symbols work on Rails versions in CI because the relevant Cucumber feature (features/view_specs/view_spec.feature:124) passes for those versions. If symbols didn't work on a particular Rails version that feature would fail.

Copy link
Member

Choose a reason for hiding this comment

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

features/view_specs/view_spec.feature:124

Fair enough. For some reason, I failed to find this feature.

Wondering if it works equally fine with variant and locale.

For the reference, a discussion about format.to_sym.
This discussion has an argument "if they don't support symbols, it's a Rails bug, and people using older versions of Rails, they should stick to older versions of rspec-rails". We've started adhering to semver since then, and currently we officially support Rails 5.2 and soft-support 5.0 and 5.1 (please correct if I'm wrong with this again). From my perspective, it would be quite irresponsible to break the current rspec-rails 5.0.x for users of Rails 5.
Even though we claim that:

According to RSpec Rails new versioning strategy use:
rspec-rails 5.x for Rails 6.x.
rspec-rails 4.x for Rails from 5.x or 6.x.

and

5.0.0 / 2021-03-09

  • Drop support for Rails below 5.2.

We have a release strategy @JonRowe has described here that I've recently violated with the merge of that PR, hopefully without any breaking impact for the users.
For the future researchers, it might be surprising that Rails 7.0 compatibility support appears in rspec-rails 5.0.3/5.1.0 releases that target Rails 6.0 and 6.1.

I suggest branching rails-7-0-dev from main, and merging this PR and #2552 to it.
When we release 5.0.3 and 5.1.0, it can be merged back to main, and it will be time for rspec-rails 6.0.

@JonRowe Does this make sense?

Copy link
Contributor

Choose a reason for hiding this comment

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

For what it's worth, that approach sounds reasonable to me.

That said I'm not sure why there's a "soft support" for Rails 5 and 5.1. If you want to support them, why not just add them to CI? If they can't be added to CI, are they actually supported?

It's worth noting that PR #2552 + #2546 has only 2 code changes of very limited scope that could end users of spec-rails:

  1. The symbol change for the handler argument ("erb" becomes :erb before being passed to ActionView::Base). This only becomes relevant in a particular type of view spec.
  2. The changes to argument handling for Action Mailer matches (in my view this is the far more likely to break item between Rails versions)

On top of these changes, this PR symbolizes a few other values passed to ActionView::Base.

That's a very small, tightly scoped set of changes. So the potential blast radius and risk of impact on end users is very limited.

Copy link
Member

Choose a reason for hiding this comment

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

if you want to support them, why not just add them to CI

We don't want to support them, and we don't want to break them. The reason is to drop some burden, mostly branching in specs:

Actually, by looking at the former, there are two changes that presumably break Rails 5.0 support. But 5.1 may still work.

PR #2552 + #2546 has only 2 code changes of very limited scope

Right 👍
Please don't get me wrong with my rant above, I'm all for merging both ASAP and releasing this as 6.0.0-pre without bothering with 5.0.3 and 5.1.0.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess I just consider "soft support" very harmful because it adds ambiguity and discussion to almost any change, without any kind of authoritative answer. If it's in CI, then you know you didn't break it (or you broke it and need to fix it). Saving a few branches is in my view not worth the ambiguity on nearly every material PR. In my experience it sows up work (and open source work) substantially.

But I'm not a maintainer here, so I'll of course defer to the library maintainers.

end
end