Skip to content
This repository has been archived by the owner on Jul 9, 2024. It is now read-only.

Rails 5 support #83

Merged
merged 13 commits into from
Dec 13, 2017
Merged

Rails 5 support #83

merged 13 commits into from
Dec 13, 2017

Conversation

zubin
Copy link
Contributor

@zubin zubin commented Dec 12, 2017

Context

Currently the gem depends on rails < 5.

Changes

  • Require rails versions 4 or 5 (drop rails 3 support).
  • Adjust code to be compatible with rails 5.

@zubin zubin requested a review from shevaun December 12, 2017 19:36
@zubin zubin changed the base branch from specify-supported-rails-versions to master December 12, 2017 22:41
guide.gemspec Outdated
@@ -14,7 +14,7 @@ Gem::Specification.new do |s|

s.files = Dir["{app,config,db,lib}/**/*", "LICENSE", "Rakefile", "README.rdoc"]

s.add_dependency "rails", ">= 3.1", "< 5"
s.add_dependency "rails", ">= 3.1"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we want to add a < 6 clause here, to help us in the next upgrade? :)

@@ -48,7 +48,11 @@
end

it "does not render the show template" do
expect(response).not_to render_template(:show)
if Rails.version < '5'
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One idea is we make this version (0.4.0) only compatible with Rails 5, and projects on Rails 4 can continue to use 0.3.2. What do you think?

Copy link
Contributor Author

@zubin zubin Dec 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shevaun I like that idea, but the only downside is that we wouldn't be able to merge the Gemfile change into master (on Market).

Maybe minimum version should be 4 (since we're using around_action now).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's a good point. Yep I'm happy to make this version 4+ compatible!

def get(action, params = {})
super action, params: params
end
end
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is interesting. Might end up using this in the marketplace app instead of changing every controller spec! Although, I imagine we'll need to do the change at some point

@@ -0,0 +1 @@
/Users/zubin/src/envato/guide/spec/test_apps/shared/app/assets/javascripts/application.js
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what are these files and why do they have your filepath in them?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shevaun These are symlinked from spec/test_apps/shared/app.

I intended to use relative symlinks but apparently did not.

Copy link

@shevaun shevaun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks!

@zubin zubin merged commit 8272b18 into master Dec 13, 2017
@zubin zubin deleted the rails-5 branch December 13, 2017 23:18
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants