-
Notifications
You must be signed in to change notification settings - Fork 207
Fix installation for gem #130
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
Conversation
+ freeze rails version upto 5.1.99
Gemfile
Outdated
gem 'ransack-mongoid', github: 'activerecord-hackery/ransack-mongoid' | ||
|
||
# Test app stuff | ||
|
||
gem 'rails', '~> 5.1' | ||
gem 'rails', '<= 5.1.99' # ransack newer than 1.8.6 causes problems in rails 5.2+ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be better to specify this as:
gem 'rails, '< 5.2'
?
'5.1.99' is not the highest possible '5.1.x' number after all. (5.1.100 is higher for example)
8cef9bb
to
66cddd5
Compare
LGTM. I understand why devise is not included. Thanks! |
You could also add 'kaminari-mongoid' since it's required for index pages. 🎉 |
+ freeze rails version up to version 5.2 + kaminari-mongoid in requirements for index pages
5b09fa5
to
af51995
Compare
@deivid-rodriguez could you have a look at this? Though I don't know if we still need it. |
I think ransack mongoid support seems to have been separated to a separate gem: https://github.com/activerecord-hackery/ransack-mongoid. Sorry I can't be of much help but it's been a long time since I don't use mongodb. I only commented the other PR because I came across it, and it seemed straightforward. |
# Conflicts: # Gemfile
i've tried without ransack declaration and without rails freeze below 5.2, and without ransack tests are okay, but on newer rails from 5.2 it has ~14 failed specs. also, locking on rails <5.2 automatically locks ransack upto 1.8.6 too, so, i think, this PR is required |
I just noticed now that this gem is alread in the Gemfile. But is should probably be in the gemspec, just like Ideallysupport for Rails 5.2 and newer versions of ransack should be added, 14 failures does not seem much. But if nobody is able to do that job, then I agree limiting the dependencies to make the current status more clear is better. |
it is around a half of all specs in test app. and also i've tried earlier these all versions of gem in our production app, activeadmin was unworking. okay, later i will try to fix this lock on rails and move this dependencies to gemspec. |
You're right, and even if it was only a small number of specs, it's not only about the number, but the importance of the specs and the difficulty to solve them. I didn't mean to say this is an easy job, my apologies!
Great, also you can take the chance to rebase the PR and fix the merge conflicts that have arised. |
i figured out. |
Nice catch I agree with your plan. Once we have a working version as long as |
okay, then later on this week (tomorrow probably) i'll do this |
closed as non-relevant, proceed to #133 |
due to issue #129 needs to freeze ransack version. ransack from version 1.8.7 is requiring active_record.
also ransack 1.8.6 is not supporting rails 5.2+, so freeze version for rails in test_app