Skip to content

Conversation

@kobsy
Copy link
Contributor

@kobsy kobsy commented Dec 13, 2021

Summary

This builds upon the work that Luke did in #7 but addresses some under-the-hood changes to ActiveRecord that took place between Rails 6.0 and Rails 6.1. It also moves CI from Travis (not running anymore without a config update) to GHA (which is free for open-source projects like this). 🙂

Closes #7

@kobsy
Copy link
Contributor Author

kobsy commented Dec 13, 2021

It doesn't look like I can request a review, but would you mind giving it a look when you get the chance, @boblail? 🙂

@boblail boblail self-requested a review December 23, 2021 16:37
Copy link
Owner

@boblail boblail left a comment

Choose a reason for hiding this comment

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

LGTM

I've removed Travis from the required status checks in Branch Protection.

Should we consider this an alternative to #7? (It looks it doesn't just build on the other PR but squashes some commits too?)

end

def types_method
before_rails61? ? :types : :klass
Copy link
Owner

Choose a reason for hiding this comment

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

I'd expect this to work — does it?

Suggested change
before_rails61? ? :types : :klass
ActiveRecord::VERSION < "6.1" ? :types : :klass

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That didn't work, but ActiveRecord.version < Gem::Version.new("6.1") did. 🙂

@kobsy
Copy link
Contributor Author

kobsy commented Jan 3, 2022

Thanks for taking a look, @boblail. I updated the version check to be a little more elegant, per your suggestion.

This would be intended to supersede #7, as it makes a few additional changes to ensure the gem is Rails 6.1+ ready.

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