codenoble / cache-crispies

Speedy Rails JSON serialization with built-in caching
MIT License
156 stars 16 forks source link

Setting railties gem upper limit to be less than 7.0 #39

Closed uxxman closed 3 years ago

uxxman commented 3 years ago

Since rails 6.2 has been dropped, the next rails version will be 7.0. This PR relaxes the upper limit to 7.0 so projects that are trying latest rails 7-alpha can still use cache-crispies.

Flixt commented 3 years ago

Do you think we should add the Rails 7 alpha here https://github.com/codenoble/cache-crispies/blob/master/Appraisals so we get tests running against version 7?

uxxman commented 3 years ago

@Flixt since rails alpha version has not been releases yet, its not possible to install railties directly. or can we install railties directly from github?

Flixt commented 3 years ago

It should be possible to do something like:

appraise "rails-edge" do
  gem 'railties', github: 'rails/rails', branch: 'master'
end

I don't know if @adamcrown would want to have that dependency, but I think it would be necessary, if we want to support the rails edge version. Rails 6.2 was not supported by cache crispies either (it was < 6.2)

I, personally, am against this change. I think it is not a good idea to do that until the Rails team releases at least an alpha. That would be the first version I'd bump the gemspec for. In any other case you should fork cache crispies and use your fork at your own risk until the version is officially supported. But @adamcrown needs to make that decision.

uxxman commented 3 years ago
appraise "rails-edge" do
  gem 'railties', github: 'rails/rails', branch: 'master'
end

This doesn't work since 'rails/rails' is not the github repo for gem 'railties'. It's a monorepo setup.

I don't know if @adamcrown would want to have that dependency, but I think it would be necessary, if we want to support the rails edge version. Rails 6.2 was not supported by cache crispies either (it was < 6.2).

This PR does not dictate that rails 6.2 or rails 7.0 is supported. It just replaces 6.2 (A never going to exists version) with 7.0 (an actual version that will exist in future).

Flixt commented 3 years ago

Right, then probably this would work:

appraise "rails-edge" do
  gem 'railties', github: 'rails/rails', branch: 'master', glob: 'railties/railties.gemspec'
end

This PR does not dictate that rails 6.2 or rails 7.0 is supported. It just replaces 6.2 (A never going to exists version) with 7.0 (an actual version that will exist in future).

I think that is exactly what pinning a version in the gemspec does, it enforces the versions the gem developer knows the gem works with and, even more important, that he/she wants to support. But as I said, that is just my opinion :)

The change you made here actually makes a difference. Before 6.1.x was the highest supported railties version. With this PR the top most allowed version is 7.

uxxman commented 3 years ago

I think that is exactly what pinning a version in the gemspec does, it enforces the versions the gem developer knows the gem works with and, even more important, that he/she wants to support. But as I said, that is just my opinion :)

The change you made here actually makes a difference. Before 6.1.x was the highest supported railties version. With this PR the top most allowed version is 7.

That's completely incorrect. The PR doesn't add rails 7 support. You are missing what "< VERSION" means and how it works.

Previously we had,

'railties', '>= 5.0.0', '< 6.2'

# Any version of railties equal to or greater than 5.0.0 and less than 6.2 is supported.

Since, there is not going to be any 6.2, this PR only says that

'railties', '>= 5.0.0', '< 7.0'

# Any version of railties equal to or greater than 5.0.0 and less than 7.0 is supported.

The added benefit is if someone wants to experiment with rails master, (for trying out the new ActiveRecord Encryption or any new feature or to even run tests against rails master just to be ready for the update), they can do so without the bundler failing.

And Appraisal test block is also not needed since we never had the test run against pre 6.2 in the first place.

Flixt commented 3 years ago

You are right and I was wrong, I was probably mislead by the title to "lax" the constraint. My apologies.

adamcrown commented 3 years ago

Thanks @uxxman . This looks good to me. Do you need a new gem version rolled out for this?

uxxman commented 3 years ago

@adamcrown v 1.3.1 would be appreciated. If you are working on other features and want to release it with them, its also ok, we can use 'master' till then.

adamcrown commented 3 years ago

@uxxman I went ahead and pushed out 1.3.1. It's easy enough to do and I don't know when the next release will be.

uxxman commented 3 years ago

@adamcrown thank you ☺️