search

haml / html2haml

Convert HTML and HTML+Erb to Haml.
MIT License
259 stars 55 forks source link

Loosen nokogiri dependency #73

Closed ghost closed 7 years ago

ghost commented 7 years ago

I loosen nokogiri dependency because i want to use nokogiri 1.7.0 or above. Also, I add test target Rubies.

Reference

This PR is related #72.

benhutton commented 7 years ago

:+1:

Nokogiri 1.7.0 includes the removal of a very annoying deprecation warning: https://github.com/sparklemotion/nokogiri/commit/5b360b8bab3149c6ca706e6ac8321496ace2e6cb

For example, here's the output of my specs right now:

image

amatsuda commented 7 years ago

Firstly, switching dependency version in the gemspec does not work. The gemspec in the repo will not be installed to the users' computers as it is. The gemspec is used to generate the metadata in the gem package. So the nokogiri version statement will actually be evaluated when the gem owner is compiling a gem package. Not when the users are installing or loading the gem.

We can never control the users' runtime dependency gem versions. The only thing we can do here is to loosen the dependency version to accept all supported versions, and then specify "per ruby version" dependencies in Gemfile for the CI to pass. That is why I showed you an example of switching the gem version in Gemfile in the previous PR.

amatsuda commented 7 years ago

Secondly, I'm not sure if the "Fix test" approach is right. If only jruby + nokogiri 1.7 performs differently from other platforms, that can be considered a nokogiri or jruby bug. In fact we're seeing exactly the same problem also in rails, https://travis-ci.org/rails/rails/jobs/188281629 and we're still not sure if it's a bug or nokogiri's intentional specification change.

amatsuda commented 7 years ago

Thirdly, please squash your four "Test against *" commits into one.

ghost commented 7 years ago

Thank you for appreciate comment 😆 I have do not enough time to do on weekdays, I will try on a holiday.

ghost commented 7 years ago

Secondly, I'm not sure if the "Fix test" approach is right. If only jruby + nokogiri 1.7 performs differently from other platforms, that can be considered a nokogiri or jruby bug. In fact we're seeing exactly the same problem also in rails, https://travis-ci.org/rails/rails/jobs/188281629 and we're still not sure if it's a bug or nokogiri's intentional specification change.

Thank you for information. I understand my appoach is not good. I will drop "Fix test" commit.

amatsuda commented 7 years ago

@takiy33 Thanks!