k0kubun / hamlit

High Performance Haml Implementation
https://rubygems.org/gems/hamlit
Other
981 stars 59 forks source link

Add Ruby 2.4 to test matrix. #97

Closed connorshea closed 7 years ago

connorshea commented 7 years ago

Happy holidays!

connorshea commented 7 years ago

Until rails/rails@47c4924 is released in Rails 4.2.8, Ruby 2.4 won't work with it.

k0kubun commented 7 years ago

We can drop json dependency without waiting ActiveSupport 4.2.8. rack-lineprof has rack ~> 1.5 dependency and it locks Rails to 4.x. lineprof gem is useless and it's okay to remove that from gemspec :)

connorshea commented 7 years ago

@k0kubun for some reason I was thinking this used rails for tests, whoops :D Let's see if the tests pass now.

connorshea commented 7 years ago

@k0kubun https://github.com/k0kubun/hamlit/blob/master/hamlit.gemspec#L34

rails is used as a development dependency, looks like.

k0kubun commented 7 years ago

Yes it's intentional. We can drop json dependency without waiting ActiveSupport 4.2.8 by using ActiveSupport 5.

And we can avoid the last CI failure like this https://github.com/k0kubun/rack-user_agent/commit/b2ca61332e19f6972465f92a50eb2b84c4471f29.

connorshea commented 7 years ago

@k0kubun in that case we'd either have to drop Ruby 2.1 from the test matrix or use separate gemfiles, which do you prefer?

k0kubun commented 7 years ago

There is no need to drop Ruby 2.1 if you use https://github.com/k0kubun/rack-user_agent/commit/b2ca61332e19f6972465f92a50eb2b84c4471f29. Even Ruby 2.0.0 is supported in that repository https://github.com/k0kubun/rack-user_agent/blob/b2ca61332e19f6972465f92a50eb2b84c4471f29/.travis.yml (though it should be removed now).

connorshea commented 7 years ago

@k0kubun but Rails 5 also requires Ruby 2.2.2 or greater, so Ruby 2.1 still wouldn't work. Unless I'm misunderstanding?

k0kubun commented 7 years ago

If you specify rack < 2, it also means that you lock Rails to 4.x and it doesn't require Ruby >= 2.2.2.

connorshea commented 7 years ago

@k0kubun oh I see, okay, that should work. It seems like it'd be simpler to use separate gemfiles though, no?

k0kubun commented 7 years ago

I agree that it's simpler but how do you avoid duplication of other gems like minitest-line, pry-byebug in Gemfiles? They are not depended by any code in this repository but I often use it and add it to Gemfile. And writing the same gem list to separated files is lengthy.

Anyway, to review, please modify Gemfile as you like.

k0kubun commented 7 years ago

In addition, separating Gemfile is not so normal and it should have some comment describing the reason. I prefer https://github.com/k0kubun/rack-user_agent/commit/b2ca61332e19f6972465f92a50eb2b84c4471f29 because the code is descriptive about the reason and doesn't need a comment. And it's easy to follow the dependency changes.

connorshea commented 7 years ago

Rails 4.2.8 should be coming out soon, so I'll just wait for that.

As for the separate gemfiles, that's not too uncommon, carrierwave and devise do it, for example.

k0kubun commented 7 years ago

I do that if it's really necessary, i.e. testing multiple Rails in one repository. Hamlit is not Rails plugin and I've thought there's no need to test multiple Rails versions. But if your intention is to test against both ActionView 4.2 and 5.0, it should be separated.

Rails 4.2.8 should be coming out soon, so I'll just wait for that.

You mean you want to use Rails 4 for ever? Because https://github.com/kainosnoema/rack-lineprof/pull/4 is merged, Rails won't be locked to 5 if rack-lineprof > 0.0.3 is released.

k0kubun commented 7 years ago

Taking over your patch. Thank you for your contribution!