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

Fix compatibility with ActiveRecord 7.2 #535

Merged
merged 3 commits into from
Aug 11, 2024

Conversation

fatkodima
Copy link
Contributor

@fatkodima fatkodima commented Jul 1, 2024

The interface for Or arel node was changed in rails/rails#51492.
This currently prevents our upgrade to rails 7.2.

Fixes #536.

.map { |feature_name, _feature_options| feature_for(feature_name).conditions }

# https://github.com/rails/rails/pull/51492
if ActiveRecord.version >= Gem::Version.new("7.2.0.beta1")
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is great! I'm wondering, should we change this to check Arel's version instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, because the arel version hasn't changed in 6 years (from git blame).

@nertzy
Copy link
Collaborator

nertzy commented Jul 11, 2024

Also it looks like the undercover code coverage tool is getting triggered for the new branch not having code coverage on both sides, but that doesn't make sense in this case so I'll have to figure out a way to improve that gate or disable it here.

@fatkodima
Copy link
Contributor Author

I once was convinced that code coverage tools are pretty useless, especially for caring too much about its numbers (if it becomes higher/lower or should be 99%). 🤷
Imo, only like running it locally for yourself to verify that you actually tested what you wanted is useful.

@fatkodima
Copy link
Contributor Author

@nertzy Sorry for pinging, but can you reconsider this PR and release a new version with the fix?

@nertzy nertzy merged commit a2c3944 into Casecommons:master Aug 11, 2024
40 checks passed
@nertzy
Copy link
Collaborator

nertzy commented Aug 11, 2024

Thanks! I updated the implementation to do a single check at require time, and based it on a direct arity check of the constructor instead of using the version.

Looks great, thanks for this fix!

@fatkodima fatkodima deleted the fix-for-rails-7.2 branch August 11, 2024 21:56
@fatkodima
Copy link
Contributor Author

Thank you ❤️

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.

Rails 7.2: wrong number of arguments (given 2, expected 1)
2 participants