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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expand slim version support to latest (v3.0.1 at present). #602

Merged
merged 1 commit into from Jan 12, 2015

Conversation

@cwest
Copy link

cwest commented Jan 4, 2015

Tests passed locally. Run against a Rails 4.2 and Rails 4.1 projects; Got expected results.

I'm not claiming a perfect understanding but things seem to be in good working order. Please let me know what I missed. 馃槃

Fixes #601.

@cwest
Copy link
Author

cwest commented Jan 4, 2015

Please review @ramaboo, @presidentbeef.

@cwest cwest force-pushed the cwest:t-upgrade-slim branch from acc9bc3 to 8de1dbb Jan 4, 2015
@cwest
Copy link
Author

cwest commented Jan 4, 2015

Updated to consider our RUBY_VERSION to maintain consistent slim support.

@cwest cwest force-pushed the cwest:t-upgrade-slim branch from 8de1dbb to 409d220 Jan 4, 2015
@presidentbeef
Copy link
Owner

presidentbeef commented Jan 4, 2015

Should probably be RUBY_VERSION < "1.9.0" or something similar just for safety.

@presidentbeef
Copy link
Owner

presidentbeef commented Jan 4, 2015

Also I'm not sure it needs to be quite so cautious on the Slim version, or else this will need to change again at the next Slim release.

@cwest cwest force-pushed the cwest:t-upgrade-slim branch from 409d220 to cf0a526 Jan 4, 2015
@cwest
Copy link
Author

cwest commented Jan 4, 2015

I agree. I also think the ternary operator doesn't convey the reason for the check, so I've made it a little more clear.

@cwest
Copy link
Author

cwest commented Jan 4, 2015

I kept it cautious based on #353 (comment).

If you're cool with being more forgiving so am I. Update forthcoming. 馃槃

@cwest cwest force-pushed the cwest:t-upgrade-slim branch 2 times, most recently from e12cba2 to eabeaeb Jan 4, 2015
@cwest cwest changed the title Expand slim support to 3.0.0 and 3.0.1 explicitly Expand slim version support to latest (v3.0.1 at present). Jan 4, 2015
@presidentbeef
Copy link
Owner

presidentbeef commented Jan 5, 2015

Sadly, I don't think this will work because the RUBY_VERSION is checked at gem creation time, not at install.

@cwest
Copy link
Author

cwest commented Jan 6, 2015

Heh. That'd be a problem indeed. :-)

@bf4
Copy link
Contributor

bf4 commented Jan 6, 2015

That's true. The best you could do is in to add a post_install message to the gemspec like

  if File.exist?('UPGRADING.md')
    gem.post_install_message = File.read('UPGRADING.md')
  end

and in the lib/brakeman.rb do something like:

# Slim 3.0.0 dropped support for Ruby 
if RUBY_VERSION < "1.9.0"
  gem "slim", ">=1.3.6", "<3.0"
else
  gem "slim", ">=1.3.6"
end
require 'slim'

Why is the dependency so open-ended?

@cwest
Copy link
Author

cwest commented Jan 6, 2015

So we have an issue here and @bf4 is right. I wanted to change the versions during install time but that seems to be a thing rubygems doesn't provide. I'm going to give his suggestion a whirl.

@cwest cwest force-pushed the cwest:t-upgrade-slim branch 2 times, most recently from 0533dfd to fbe87cd Jan 6, 2015
@presidentbeef
Copy link
Owner

presidentbeef commented Jan 6, 2015

Tempting to just leave slim out of dependencies and let bundler or users handle it themselves.

@cwest
Copy link
Author

cwest commented Jan 6, 2015

@presidentbeef I hear you. The difficulty is that we want to support 1.8.7 but they don't. I think I have a way to do that, but bundler doesn't honor it during tests.

We could also go with the optional dependency and the raise if someone wants to use it but doesn't install it (along with an UPGRADING.md warning).

@cwest
Copy link
Author

cwest commented Jan 6, 2015

I got install working but broke Travis. Will think on it more.

@presidentbeef
Copy link
Owner

presidentbeef commented Jan 6, 2015

Right. As you probably saw, Brakeman already runs fine with optional dependencies (changes were made a while ago to support brakeman-min).

@cwest
Copy link
Author

cwest commented Jan 6, 2015

I'll go in that direction after all. And after my commute home. (If you beat me to it with your own PR I won't be hurt. Promise.) 馃槃

@bf4
Copy link
Contributor

bf4 commented Jan 6, 2015

Did you try tilt?

@cwest
Copy link
Author

cwest commented Jan 6, 2015

I haven't.

@cwest cwest force-pushed the cwest:t-upgrade-slim branch 4 times, most recently from 7b4af67 to efa24aa Jan 7, 2015
@cwest cwest force-pushed the cwest:t-upgrade-slim branch from efa24aa to 24a9654 Jan 7, 2015
@cwest
Copy link
Author

cwest commented Jan 7, 2015

And we're back. I think I like this solution. The project Gemfile specifies slim, and can do so on a per-ruby basis, but the gemspec no longer does.

Tested this on a Rails 4.2 app (Ruby 2.2.0) using slim 3.0.1 after installing it and upgrading my Gemfile.lock. Worked as expected!

@presidentbeef
Copy link
Owner

presidentbeef commented Jan 7, 2015

Essentially this is the same as removing Slim from from the gem dependencies, which is fine I suppose.

@cwest
Copy link
Author

cwest commented Jan 7, 2015

Right, but keeps it available for testing and development (within bundler contexts).

@cwest
Copy link
Author

cwest commented Jan 7, 2015

This is very much how tilt deals with things, as @bf4 was trying to point out, I think. 馃槃

BTW, my apologies for exposing you to some of the more exotic attempts I made. It was pretty rough there for a bit.

@cwest
Copy link
Author

cwest commented Jan 9, 2015

Let me know if there's anything I missed on this PR? I'd be nice for Slim and Rails 4.2 users to have the latest Brakeman. 馃槃

README.md Outdated

$ gem install slim --version '~> 2.0.0'

If you're on a more recent Ruby this step is _not_ required.

This comment has been minimized.

Copy link
@presidentbeef

presidentbeef Jan 9, 2015

Owner

This is a little misleading...you need to install slim manually if you want to process slim templates, either way.

Casey West
At the time of this commit that's slim v3.0.1.

That is, except for Ruby 1.8.7 installations. Slim dropped support[^0] for Ruby
1.8.7 in their 3.0.0 release. This commit allows Brakeman to move forward with
slim support on all supported later Ruby versions.

To do this accurately we must install an appropriate version of Slim by
inspecting `RUBY_VERSION` at install time. We do that by using the native
extensions hooks available as part of gem installs. Ours is modeled after an
example on GitHub[^1].

[^0]: https://github.com/slim-template/slim/blob/master/CHANGES#L12
[^1]: https://github.com/mmzyk/gem_dependency_example/tree/master/gem_dep_example
@cwest cwest force-pushed the cwest:t-upgrade-slim branch from 24a9654 to 2e44182 Jan 11, 2015
@cwest
Copy link
Author

cwest commented Jan 11, 2015

Docs update to look like this:

For Slim Users

Slim v3.0.0 dropped support for Ruby 1.8.7. Install a version of slim compatible with your Ruby.

Ruby Version Gemfile Command Line
Ruby 1.8.7 gem 'slim', '< 3.0' $ gem install slim --version '< 3.0'
Ruby 1.9+ gem 'slim' $ gem install slim
presidentbeef added a commit that referenced this pull request Jan 12, 2015
Expand slim version support to latest (v3.0.1 at present).
@presidentbeef presidentbeef merged commit 13bc8db into presidentbeef:master Jan 12, 2015
1 check passed
1 check passed
continuous-integration/travis-ci The Travis CI build passed
Details
@presidentbeef
Copy link
Owner

presidentbeef commented Jan 12, 2015

Thanks for your patience!

@cwest
Copy link
Author

cwest commented Jan 12, 2015

@presidentbeef++ 馃槃

@Lordnibbler
Copy link

Lordnibbler commented Jan 13, 2015

@presidentbeef When can we expect a new release on rubygems for this?

@presidentbeef
Copy link
Owner

presidentbeef commented Jan 13, 2015

@Lordnibbler I don't like to be too specific about release dates, but I do expect another release relatively soon.

Repository owner locked and limited conversation to collaborators Feb 16, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can鈥檛 perform that action at this time.