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

[chore] Relax rack dependency #25

Merged
merged 1 commit into from Feb 21, 2017
Merged

[chore] Relax rack dependency #25

merged 1 commit into from Feb 21, 2017

Conversation

chytreg
Copy link
Contributor

@chytreg chytreg commented Feb 16, 2016

I use the gem with rails 5 which works on rack 2.0 only

@swistaczek
Copy link

Hey @chytreg , please try to fix travis build.

@olleolleolle
Copy link
Contributor

olleolleolle commented Jul 1, 2016

(That build error, isn't that transient? It ran 5 months ago, with your typical "network glitch" output. If there were a re-try button, I'd push it, but only the owner can push that button.)

@chytreg
Copy link
Contributor Author

chytreg commented Jul 18, 2016

Unfortunately, there is a problem with ruby version. Rack 2, requires >= 2.2.2
https://github.com/rack/rack/blob/master/rack.gemspec#L29
I'm not sure how to resolve this problem, any guide how to setup it properly would be useful.

@olleolleolle
Copy link
Contributor

@chytreg
Copy link
Contributor Author

chytreg commented Jul 18, 2016

The question is how to support new features for the older version. If we add runtime dependency this will be breaking change (only rack >=2, only ruby >=2.2.2). Probably we should release some major version of Envied WDYT, and adjust test runner to skip ruby versions prior than 2.2.2

@olleolleolle
Copy link
Contributor

olleolleolle commented Jul 18, 2016

Let me suggest an if in the gemspec.

if RUBY_VERSION < '2.2.2'
  spec.add_dependency "rack", "~> 1.4"
else
  spec.add_dependency "rack", ">= 1.4"
end

It seems only the parse_query from Rack::Utils is used.

::Rack::Utils.parse_query(str)

@javierjulio
Copy link
Contributor

Would the suggestion @mpalmer made which uses < 3, >= 1.x (I've seen this elsewhere for rack) be a better choice? If so I'd be happy to submit a PR to get this Rails 5 ready. Love Envied. ❤️

@chytreg
Copy link
Contributor Author

chytreg commented Jul 22, 2016

@javierjulio & @mpalmer I've managed to make it work on any ruby version.
Should I squash my commits before the PR is going to be merged?

- allows to work with Rails 5
- set proper rack dependency based on ruby version
- solve problem with undefined method `spec' for nil:NilClass travis-ci/travis-ci#5239
- test against ruby 2.2.0 and 2.3.0
@chytreg
Copy link
Contributor Author

chytreg commented Jul 22, 2016

FYI it's rebased and ready to merge

@javierjulio
Copy link
Contributor

@chytreg thanks! @eval any chance this can get reviewed so Envied is Rails 5 ready?

@rtlong
Copy link

rtlong commented Sep 22, 2016

This is great! @eval, if you could get 'round to merging this, it would make my day!

@bradfeehan
Copy link

It'd be great to get this merged, or a list of remaining tasks to get it across the line. Thanks for this tool <3

@javierjulio
Copy link
Contributor

I've asked @eval to consider a set of releases for Envied in #29 and offered to take ownership of getting them out if he wants. I included this PR as the first release to go out as a patch update. Other issues have been organized into new minor releases. I would love feedback from the rest of you.

@javierjulio javierjulio merged commit d7ae603 into eval:master Feb 21, 2017
@javierjulio
Copy link
Contributor

Thanks so much for doing this @chytreg! ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants