Skip to content

[chore] Relax rack dependency - #25

Merged
javierjulio merged 1 commit into
eval:masterfrom
monterail:master
Feb 21, 2017
Merged

[chore] Relax rack dependency#25
javierjulio merged 1 commit into
eval:masterfrom
monterail:master

Conversation

@dgertych-monterail

Copy link
Copy Markdown

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

@swistaczek

Copy link
Copy Markdown

Hey @chytreg , please try to fix travis build.

@olleolleolle

olleolleolle commented Jul 1, 2016

Copy link
Copy Markdown
Contributor

(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.)

@dgertych-monterail

Copy link
Copy Markdown
Author

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
Copy Markdown
Contributor

@dgertych-monterail

Copy link
Copy Markdown
Author

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

olleolleolle commented Jul 18, 2016

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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. ❤️

@dgertych-monterail

Copy link
Copy Markdown
Author

@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
@dgertych-monterail

Copy link
Copy Markdown
Author

FYI it's rebased and ready to merge

@javierjulio

Copy link
Copy Markdown
Contributor

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

@rtlong

rtlong commented Sep 22, 2016

Copy link
Copy Markdown

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

@bradfeehan

Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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

Development

Successfully merging this pull request may close these issues.

7 participants