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

Allows Rails 8, updates sqlite in Gemfile to match what CI runs #6091

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rjacoby
Copy link

@rjacoby rjacoby commented Jan 28, 2025

Summary

Bumps Gemfile and solidus_core.gemspec up to support Rails 8.0. Makes minor changes to update code and tests to deal with Rails 8 changes.

This required bumping sqlite3 above 2.0 to get specs to pass; note that the circleci config already required this and was not testing Rails 7.0 for sqlite likely due to this compatibility mismatch.

Checklist

Check out our PR guidelines for more details.

The following are mandatory for all PRs:

The following are not always needed:

  • 📖 I have updated the README to account for my changes.
  • 📑 I have documented new code with YARD.
  • 🛣️ I have opened a PR to update the guides.
  • ✅ I have added automated tests to cover my changes.
  • 📸 I have attached screenshots to demo visual changes.

@rjacoby rjacoby requested a review from a team as a code owner January 28, 2025 18:48
@github-actions github-actions bot added the changelog:solidus_core Changes to the solidus_core gem label Jan 28, 2025
@github-actions github-actions bot added the changelog:solidus_api Changes to the solidus_api gem label Jan 28, 2025
@rjacoby
Copy link
Author

rjacoby commented Jan 28, 2025

@tvdeyen I've got all the tests passing locally, on all 3 databases. Seems like test flake on that one circle run, but I don't have perms to rerun it.

Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

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

@rjacoby Thank you! re-running specs, I left a question, let me know what you think about it.

@rjacoby
Copy link
Author

rjacoby commented Jan 30, 2025

@kennyadsl Updated with your suggestion.

Copy link

codecov bot commented Jan 30, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.70%. Comparing base (ca1ae4d) to head (0e9bd91).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6091      +/-   ##
==========================================
- Coverage   92.42%   88.70%   -3.73%     
==========================================
  Files         389      829     +440     
  Lines        8005    17992    +9987     
==========================================
+ Hits         7399    15959    +8560     
- Misses        606     2033    +1427     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

Thanks. Very much appreciated. Would you mind to rebase with main (and not merge) and squash the fixup into the introducing commit?

@tvdeyen
Copy link
Member

tvdeyen commented Jan 31, 2025

@rjacoby Rubocop needs your attention as well 🙏🏻

@fthobe
Copy link
Contributor

fthobe commented Jan 31, 2025

OMG, this is happening much sooner than expected. Thank you so much guys.

Only send content to tags that support it per Rails 8 deprecations. See: https://github.com/rails/rails/blob/8-0-stable/actionview/CHANGELOG.md#rails-800rc1-october-19-2024

Handle Rails 8 syntax additions to the error text.
@rjacoby
Copy link
Author

rjacoby commented Jan 31, 2025

@tvdeyen Rebased, but now the project's codecov isn't happy...

Copy link

@kushp kushp left a comment

Choose a reason for hiding this comment

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

nice

Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:solidus_admin changelog:solidus_api Changes to the solidus_api gem changelog:solidus_core Changes to the solidus_core gem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants