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

Remove upper bound of Active Record #1213

Closed
wants to merge 1 commit into from

Conversation

rafaelfranca
Copy link
Contributor

@rafaelfranca rafaelfranca commented Jul 17, 2019

The way it is was didn't allow people to try paper trail with rails master to check if it work or not. Removing the upper bound doesn't assert that the gem works with all versions, it just allow people to try themselves and even open PRs fixing what is broken.

They way it is was didn't allow people to try paper trail with rails master to check if it work or not. Removing the upper bound doesn't assert that the gem works with all versions, it just allow people to try themselves and even open PRs fixing what is broken.
@jaredbeck
Copy link
Member

Hi Rafael,

The way it is was didn't allow people to try paper trail with rails master to check if it work or not.

To test compatibility with new versions of rails, I use my fork of PT, I edit the gemspec, and use gem .. path: /path/to/local in my application. I agree this is inconvenient.

Removing the upper bound doesn't assert that the gem works with all versions, it just allow people to try themselves and even open PRs fixing what is broken.

Removing the upper bound will cause people to install incompatible PT/rails pairs. Then, instead of a friendly message from bundler like "PT X doesn't work with rails Y", the user will get a confusing error, or worse, may discover the incompatibility in production. This could be a very bad experience for a beginner.

I respect your opinion greatly, but removing the upper bound sounds dangerous to me. I'll have to think carefully about this, and I'd welcome other considered opinions.

@rafaelfranca
Copy link
Contributor Author

To test compatibility with new versions of rails, I use my fork of PT, I edit the gemspec, and use gem .. path: /path/to/local in my application. I agree this is inconvenient.

Yeah, that works for testing, but the problem is that I don't want to test compatibility, I want to use in my application. My app is always running on Rails master. This upper bound basically means we have to run a fork or wait added official support, what usually takes months after the final release of Rails. That is not only inconvenient but impractical.

I also understand your concerns as well. I had the same reservations before until we change the policy on Rails itself to remove upper bounds on everything.

In theory the problem you are describing exists but in practice you get more reports of "bundler don't let me install this" than "I tried this gem with rails master and it gave me this weird message or broke my app in production". Rails does follow SemVer, it is just don't use the number in the same way, but it is very hard to Rails to break any behavior without deprecating it before. Some times we need to change the gem to remove deprecation calls but if you don't do the users will only see the deprecations message, not break their application.

Looking the last upgrades it seems only tests or gemspec where changed, what shows that Rails releases are stable for your usage at lest.

#1172
#1068

All my gems don't have upper bound and it didn't caused any compatibility report like the ones your are mentioning.

@jaredbeck
Copy link
Member

I've looked at the requirements of some popular gems. Some of the most important gems, like devise, have upper-bound requirements, but many do not. The latter practice is more common than I realized.

# Do gems constrain upper bound of most important dependency?

Analysis of latest versions, as of 2019-07-25

## Has Upper Bound

acts-as-taggable-on 6.0.0         activerecord ~> 5.0
active_model_serializers 0.10.10  activemodel < 6.1
audited 4.9.0                     activerecord < 6.1
delayed_job 4.1.7                 activesupport < 5.3
delayed_job_active_record 4.1.3   activerecord < 5.3
devise 4.6.2                      railties < 6
paranoia 2.4.2                    activerecord < 6.1
resque 2.0.0                      redis-namespace ~> 1.6

## No Upper Bound

activerecord-import 1.0.2   activerecord >= 3.2
acts_as_list 0.9.19         activerecord >= 3.0
ar-octopus 0.10.2           activerecord >= 4.2.0
carrierwave 1.3.1           activemodel >= 4.0.0
database_cleaner 1.7.0      activerecord >= 0
factory_bot 5.0.2           activesupport >= 4.2.0
friendly_id 5.2.5           activerecord >= 4.0.0
kaminari 1.1.1              activesupport >= 4.1.0
paperclip 6.1.0             activemodel >= 4.2.0
polyamorous 1.3.3           activerecord >= 3.0
ranked-model 0.4.4          activerecord >= 4.1.16
ransack 2.1.1               activerecord >= 5.0
redis-namespace 1.6.0       redis >= 3.0.4
rspec-rails 3.8.2           actionpack >= 3.0
sidekiq 6.0.0.pre1          redis >= 4.0.2

@rafaelfranca
Copy link
Contributor Author

@Edouard-chin
Copy link
Contributor

@jaredbeck
Copy link
Member

Please take a look at #1216 and let me know if it works for you.

@jaredbeck
Copy link
Member

Closed by #1216, will release in 10.3.1

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