lessonly / scim_rails

SCIM Adapter for Rails.
MIT License
68 stars 76 forks source link

Add support for ruby 3 versions #65

Closed pragya-sriharsh closed 1 year ago

pragya-sriharsh commented 1 year ago

Why?

I am working on ruby & rails upgrade stories. As this branch is restricted to ruby 2.4 version so i have modified to support > ruby 3 versions. Initially i have modified for ruby 3.2.2 version but lessonly's some gems have some dependencies on ruby 3.0.0 so i degraded from 3.2.2 to 3.0.0. But this changes is working as expected for ruby 3.0.x too.

What?

Modified files to support ruby 3 version:

Caveats

I don't know where all this gem is being used. Currently i am using it along with lessonly with this feature branch.

Testing Notes

NA

Merge Instructions

Please DO NOT squash my commits when merging

pragya-sriharsh commented 1 year ago

A small change in meta to indicate ruby versions >=2.4 and <=3.2

unixmonkey commented 1 year ago

Why cap the upper bound at all? I would bet it works fine with Ruby 3.3 and up.

pragya-sriharsh commented 1 year ago

Yes it will work with Ruby 3 and up. I just thought to mention that otherwise >=2.4 was enough.

unixmonkey commented 1 year ago

If you leave this library locked to <=3.2, then around the end of the year, when Ruby 3.3 final is released, then this gem will need another release, and potentially block people from upgrading to 3.3 if they use this as a dependency. Maybe for years until someone with commit access finally gets around to it, like has happened already.

In the small case that maybe something with Ruby 3.3 or higher does somehow "break" with this gem, I'm sure you'll hear about it, and then can work on an actual code fix, instead of re-pinning an upper bound, and re-releasing the gem every time a new Ruby is released.

Yes, I would recommend removing the upper bound entirely, at least until you know for sure a version won't work with it.

pragya-sriharsh commented 1 year ago

Okay got it @unixmonkey . Reverting this commit.

aripollak commented 1 year ago

Thanks for merging this! Would you mind releasing a new official gem version with this change included?