-
Notifications
You must be signed in to change notification settings - Fork 238
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
Add Rails 7 Support #391
Add Rails 7 Support #391
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using this branch since a few weeks, without any issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is looking great. Thank you @OskarEichler! (And everyone else who's participating.)
I haven't checked the test suite, that's why this is only a comment, but the changeset is pleasantly small. Thank you for doing the research on the new dependencies!
From what I gather, TravisCI is no longer running the tests here and there was mention to migrate to Github actions. Plus passing tests would be 👍 for this PR. This PR on my fork successfully runs a matrix of tests on Github actions - but the tests themselves fail unfortunately. Feel free to copy that .github/workflows/ruby.yml file into this PR to enable Github Action based CI testing. |
I’ve opened a PR to clean up some of the cruft in master. I hope that it helps move this PR forward. The intention is to get CI working so that this PR is a smaller and easier-reviewed change. Note that on that being merged to master this will need:
Apologies for any inconvenience - and thanks for having done this! |
Hi guys, when this could be merged? |
Hi @gonzalo-bulnes - this PR has been open for 12 months. Do you have an ETA on this or could you provide guidance on how the community should proceed? |
@pglombardo If I knew you I'd probably ask you what have you done in the past 12 months. 🤨 But I we don't know each other, so I don't. And based on that premise you seem very mistaken on the nature of our relationship. To make it clear: you're not entitled to any of my time. In addition, I don't know what makes you think I have any interest in making any of your decisions for you. 🤷♀️ Please take note that that kind of posturing is not welcome, and let's move on. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @OskarEichler,
This PR is indeed looking great 🚀 there are only a couple of details to tweak to get it ready - please don't let the big red "changes requested" icon discourage you, I really wish it wasn't that dramatic!
As I mentioned above, I've added a GitHub Actions workflow that runs the test suite. The next two steps are:
- Please rebase this PR onto the latest
master
:- that will enable to run your code in GitHub's servers
- and put us in a good position to merge the PR when ready
- Remove the version change. (I'll bump the version after the PR is merged.) Please move the change log additions in the Unreleased section (that exists in
master
), those are good. 👍
Please note that you'll need to address two separate issues in order to do the above. I've written details in comments next to the code. If anything is unclear, just let me know, I can work you through it. 🙂
Merge Master 2
@gonzalo-bulnes Thanks so much! I went ahead and merged master, resolved the conflict and made the changes suggested by you - thanks for the thorough review! Please let me know if there's anything else you'd like me to change / update. Regarding the version number I recommend potentially skipping 1.18.0 and instead using 1.18.1 or higher. Reason is, that quite a few people have been using my fork which had version 1.18.0 for the past few months, so they might not be pulling in the latest changes correct when they switch back to master as the version would remain the same 1.18.0 for them. Happy holidays 🎄 |
Note that, as-is, relying on (Support for Ruby 1.9.3 is potentially dropping too, but that's unrelated. It has to do with setting up a successful CI job, another not very significant reason to cut a v2.0.0.) |
I didn't see your comment when writing mine! :P (I didn't expect it today!) Regarding version numbers: there isn't much to choose from. The gem adheres to semantic versioning. Breaking changes (like dropping support for anything that was supported) require a major version change. When you've got a moment, take a look at this copy of your branch. I've been thinking about it and shaping what the different constraints lead to. The commits are not in order or grouped, that copy is in a very raw draft state, but the resulting file changes are worth a look. |
@gonzalo-bulnes Ohh yeah I saw the test failing for Rails 4. Had a look at your copy of the branch, and the changes look good. I guess in that case the version would probably be 2.0.0 due to the breaking changes. |
This comment was marked as off-topic.
This comment was marked as off-topic.
It turns out that the active model serializer gems are not needed at all. I've used some |
@OskarEichler I've opened #401 as a replay of your PR. I've written down the details over there for you, but I think we should continue the conversation over here. I'll proceed to merging it (which will close this PR) - can you please give a try to installing the gem from |
|
I've released v1.18.0, thank you for seeing this PR through @OskarEichler! 🎉 (I didn't wait for your thumbs up, but if anything is wrong I'll fix it.) (Regarding the version number, there shouldn't be any issues for people who are currently using your branch/fork, since the source of the gem will be different.) |
This PR adds support for Rails 7.
Closes #393