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

Added support for Rails 8 #765

Closed
wants to merge 0 commits into from
Closed

Added support for Rails 8 #765

wants to merge 0 commits into from

Conversation

basex
Copy link

@basex basex commented Nov 12, 2024

Followed the same as #752

I tested locally and works without issues.

In the future if we stop supporting Rails 2 we can check for Rails::VERSION::MAJOR. Example: Rails::VERSION::MAJOR > 6

@tagliala
Copy link
Collaborator

Hi, thanks for this PR.

Failures do not depend by this PR. I have some small changes to do to the CI, then I'll ask to rebase

@tagliala
Copy link
Collaborator

@basex please rebase, specs should pass

@basex
Copy link
Author

basex commented Nov 13, 2024

done @tagliala Thanks

@tagliala
Copy link
Collaborator

All green, let me try if I remember how to run integration tests. Did you run it locally?

@tagliala
Copy link
Collaborator

Sorry, rabl needs some other changes that are not related to this PR.

I will make the changes elsewhere and then ask to rebase or cherry-pick your commit to preserve your authoring

@tagliala
Copy link
Collaborator

tagliala commented Nov 14, 2024

Thanks,

I've performed an integration test locally against rails 8 and it worked, however we now have two options

  1. I'll cherry-pick this commit in a new PR and make the change. You will be preserved as an author of the commit, but I'm not sure that your name will appear in the automatic release notes
  2. You create a new branch on your fork, possibly with a good name, let's say rails8, then:
  • Cherry pick basex/rabl@1c45233 on that branch
  • Reset your fork master branch on this gem's master
  • Rebase rails8 on master, submit a new PR

Point 2 would also apply as best practice for future PRs. It is required to have a single clean commit to merge here (I do not have the "rebase" feature available, maybe because it's the master branch on the remote fork)

Please also add the following at the top of CHANGELOG.md

## unreleased

* Add Rails 8 compatibility (@basex)

@basex
Copy link
Author

basex commented Nov 14, 2024

Discarding the commits in my fork automatically closed this MR. New PR from another branch here #768

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