-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Allow rails 5.1 usage #856
Conversation
spec/support/http_method_shims.rb
Outdated
end | ||
end | ||
|
||
if Rails::VERSION::MAJOR >= 5 | ||
if Gem::Dependency.new('rails', '~> 5.0.0').match?('rails', Rails::VERSION::STRING) |
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.
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
spec/support/http_method_shims.rb
Outdated
|
||
def delete(path, params = nil, headers = nil) | ||
super(path, params: params, headers: headers) | ||
if Gem::Dependency.new('rails', '~> 5.1.0').match?('rails', Rails::VERSION::STRING) |
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.
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
2abf423
to
a784a81
Compare
#782 was me trying to keep things up to date for a project as well. It's kinda me backporting stuff. I need to close it and reopen with a new PR. It's just got gotten messy. Thanks for tackling this! |
Thanks @tiagoamaro! I'm not sure yet how I want to go about merging these two, but it's the focus for |
On #782, we were blocked by rails/sass-rails#400, which was merged today. Does this help us at all here? |
a784a81
to
d70263c
Compare
spec/support/http_method_shims.rb
Outdated
end | ||
end | ||
|
||
if Rails::VERSION::MAJOR >= 5 | ||
if Gem::Dependency.new("rails", "~> 5.0.0").match?("rails", Rails::VERSION::STRING) |
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.
Line is too long. [83/80]
spec/support/http_method_shims.rb
Outdated
|
||
def delete(path, params = nil, headers = nil) | ||
super(path, params: params, headers: headers) | ||
if Gem::Dependency.new("rails", "~> 5.1.0").match?("rails", Rails::VERSION::STRING) |
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.
Line is too long. [83/80]
d70263c
to
b4ba328
Compare
The tests are failing because Rails 5 and up doesn't like unversioned migrations, so I'm going to worry about that in another PR and merge this. Thanks! |
Hello,
First, thank you very much for this gem! It's extremely useful, as its code is quite impressive and clean 🎉 very nice job!
This PR is my attempt to allow Rails 5.1 usage, while keeping the entire test suite green. There are some deprecation warnings, but they're related to
database_cleaner
:Ran
bundle exec rake && bundle exec appraisal rake
and everything passed fine :)Also, I'm currently using my fork on a project in production, and it seems to be working fine.
Note: I've noticed a PR to support Rails 5.1 (#782), but it seems it's tackling more than just allowing this new Rails version to be used, so I decided to open this one.