Skip to content

Cleanup project for Rails 7+ support#3

Merged
moberegger merged 7 commits into
mainfrom
remove_rails_6_support
May 29, 2025
Merged

Cleanup project for Rails 7+ support#3
moberegger merged 7 commits into
mainfrom
remove_rails_6_support

Conversation

@moberegger

@moberegger moberegger commented May 28, 2025

Copy link
Copy Markdown

Recommended to review with white space off - most of the changes are adjustments to indentation as a result of removing several if conditions against versions of Rails. Also recommended to review commit by commit.

Support was dropped for EOL Ruby and Rails versions back in September in rails#570. Part of the motivation to do this was to address some test issues in another PR. Since then, nothing below Ruby 3.0 and Rails 7.0 run as part of the test matrix. Given that the project no longer runs validation against older versions of Ruby and Rails, compatibility can no longer be guaranteed. I think it makes sense to update the project to reflect this reality.

A summary of the changes:

  • gemspec has been updated to specify Ruby 3.0 is the minimum version, as well as specifying 7.0 as the minimum versions for both activesupport and actionview, since this is what the test matrix actually runs against.
  • Test cases have been cleaned up to remove some redundant if conditions against Ruby/Rails versions that would always be true in the test matrix. Likewise, conditions and assertions for older versions have been removed since they would no longer be running under the test matrix.
  • The railtie has been tidied up to assume Rails 7+
  • The CollectionRenderer has been refactored to trust that ::ActionView::CollectionRenderer exists, which is the case for Rails 7+. This allows us to remove the ::ActionView::PartialRenderer shim which also allows us to remove the branch of logic in JbuilderTemplate where CollectionRenderer.supported? is false, which will never be the case anymore in Rails 7+.

This also been filed upstream: rails#594

@moberegger moberegger changed the title Update gemspec for Rails 7 Cleanup project for Rails 7+ support May 29, 2025
@moberegger moberegger marked this pull request as ready for review May 29, 2025 14:47
@moberegger moberegger requested review from Insomniak47 and mscrivo May 29, 2025 14:47

@mscrivo mscrivo left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔥

end
end

# Rails 6.1 support:

@Insomniak47 Insomniak47 May 29, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Is this func still needed then?

(the each_with_info func)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

It is. It really should have said "Rails >= 6.1 support".

def collection_with_template(view, template, layout, collection)
super(view, template, layout, ScopedIterator.new(collection, @scope))
end
def initialize(lookup_context, options, &scope)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This initialize was part of CollectionRenderer - since you're removing its definition is this initialize still needed?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

CollectionRenderer is still being defined. The PR is removing the CollectionRenderer defined for < Rails 6.1.

class CollectionRenderer < ::ActionView::PartialRenderer is gone.
class CollectionRenderer < ::ActionView::CollectionRenderer remains.

CollectionRenderer was being conditionally defined via if defined?(::ActionView::CollectionRenderer)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

So this is still needed?

@Insomniak47 Insomniak47 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM w/ a few q's

@moberegger moberegger merged commit 0a67bbd into main May 29, 2025
30 checks passed
@moberegger moberegger deleted the remove_rails_6_support branch June 13, 2025 18:54
@moberegger moberegger restored the remove_rails_6_support branch June 17, 2025 02:05
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.

3 participants