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

Support ActiveSupport 4.1 #125

Closed
simeonwillbanks opened this issue May 20, 2014 · 8 comments
Closed

Support ActiveSupport 4.1 #125

simeonwillbanks opened this issue May 20, 2014 · 8 comments

Comments

@simeonwillbanks
Copy link
Contributor

ActiveSupport v4.1.0 depends upon 'minitest', '~> 5.1'.

This dependency breaks the html-pipeline build. Here is a more detailed explanation.
#123 found a temporary solution by disallowing ActiveSupport 4.1.0. or greater. A more permanent solution must be found.

@simeonwillbanks
Copy link
Contributor Author

I created an issue in rails/rails.

@jdickey
Copy link

jdickey commented May 20, 2014

So much for clean OO design; the attitude expressed in that Rails bug is pretty brusque. I suppose anybody in this position is expected to fork AS 4.0.5 or 3.2.18 and cherry-pick from upstream forever. 👎

@simeonwillbanks
Copy link
Contributor Author

Since ActiveSupport won't change their gemspec, we must explore other options.

  1. Refactor test suite to use minitest gem instead of stdlib test/unit
  2. Before the test helper requires 'test/unit', we could explicitly require the stdlib 'minitest/unit', so test/unit does not require the minitest gem.

Option 2 might be OK, but the code might be hacky.

Option 1 didn't seem reasonable, but Ruby 2.2.0 is removing stdlib test!

From ruby/NEWS:

  • lib/test/*/.rb
    • Removed because it conflicts to minitest 5, and it was just an wrapper
      of minitest 4. [Feature #9711]

Maybe option 1 is the right choice... Thoughts?

cc @jch @rsanheim

@jdickey
Copy link

jdickey commented May 21, 2014

@simeonwillbanks Agree with your assessment of both options. 👍 for Option 1

@jch
Copy link
Contributor

jch commented May 21, 2014

I love minitest, but hate that it's a gem rather than in the stdlib. That's orthogonal to this discussion though. I'm 👍 to seeing a option 1 PR with minitest.

@simeonwillbanks
Copy link
Contributor Author

Feature #9711 - "Remove test-unit and minitest from stdlib" details the removal.

Feature #9852 - "How to bundle test-unit2 and minitest5" discusses what's next.

It appears the stdlib minitest is and will be the minitest gem. Ruby trunk imports the minitest gem into the stdlib.

That said, I still think we should bundle the minitest gem and proceed with the refactor because ActiveSupport is depending upon minitest via development_dependency. The ActiveSupport minitest version doesn't match the stdlib version, and this could continue. We'll have less issues matching ActiveSupport rather than the stdlib. 😄

Thanks for the input @jdickey and @jch. I'll start a Pull Request.

@scotje
Copy link

scotje commented May 28, 2014

Is ActiveSupport really a runtime dependency still? I know the instrumentation stuff is expecting something that implements ActiveSupport::Notifications, but it doesn't seem to actually depend on it inheriting from that. Is there something else I'm missing outside of the tests?

I made a branch that removes ActiveSupport entirely (replacing the HTML fragment comparison functionality with something based on lorax) and all the unit tests still pass on 1.9.3 at least. (Happy to submit this as a PR if you like.)

ActiveSupport is just kind of a large and opinionated dependency to add to non-Rails based apps if it's not strictly necessary.

@simeonwillbanks
Copy link
Contributor Author

e00c3c9 fixes this issue.

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

No branches or pull requests

4 participants