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

Rails 7.2 compatibility #1632

Merged
merged 6 commits into from
Aug 15, 2024
Merged

Rails 7.2 compatibility #1632

merged 6 commits into from
Aug 15, 2024

Conversation

theodorton
Copy link
Contributor

Supersedes #1631

I've been running tests against various git versions of Rails for one of my projects and noticed there are a couple things breaking in shoulda-matchers. This led me to try running the shoulda-matcher tests against Rails 7.2 and I discovered some things that were no longer compatible. This PR should resolve the previously failing tests.

Let me know if you'd like me to refactor and/or reword anything here.

I understand if you want to hold off on merging this until 7.2 is officially released. I can update the Appraisal and rebase when it's out.

@theodorton
Copy link
Contributor Author

Rubocop issues should be resolved.

@matsales28
Copy link
Member

@theodorton Thanks for working on that! The overall changes look good to me. Feel free to add the new appraisal to the workflow file so we can run the CI with the new rails version.
https://github.com/thoughtbot/shoulda-matchers/blob/main/.github/workflows/ci.yml

Also, could you add specs for the counter_cache change? The syntax has changed a bit.
I think it's better to wait for the official release as if they decide to change more stuff, there might be other necessary changes.

Thanks again for working on that!

@theodorton
Copy link
Contributor Author

@matsales28 I added specs for counter_cache in 4224dec, and acceptance specs should pass with the latest commits. I'll rebase to clean up once you feel it's time to merge.

@spickermann
Copy link

Ruby on Rails 7.2.0 was officially released a few hours ago. Is there anything I can do to help to get this PR merged and deployed?

@matsales28
Copy link
Member

Hi @theodorton,
Thank you for your work on the PR! With the new Rails version now officially released, it would be great if you could continue with the adjustments.

@spickermann has also offered to help move this PR forward. If you're open to collaborating. I'm happy to assist as well—just let me know if there's anything I can do to support you. If you don't have time now to do that, please let me know and I can move this PR forward myself.

I really appreciate the effort!

@theodorton
Copy link
Contributor Author

@matsales28 I'll spend some time today to bring it up to date with the official release and see if I can resolve the failing specs. I will let you know if I need help moving it forward.

@theodorton
Copy link
Contributor Author

@matsales28 I've cleaned up the history here and believe I've resolved all the issues that surfaced from the previous build.

@matsales28
Copy link
Member

@matsales28 I've cleaned up the history here and believe I've resolved all the issues that surfaced from the previous build.

CI passed. I'll take another look at the code later, but it's looking good. If nothing extraordinary happens, I'll probably merge it today, and release a new version. Thanks again for working on this!

@matsales28 matsales28 merged commit 5c92966 into thoughtbot:main Aug 15, 2024
21 checks passed
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