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

Refinements #72

Merged
merged 9 commits into from
Jan 9, 2016
Merged

Refinements #72

merged 9 commits into from
Jan 9, 2016

Conversation

kputnam
Copy link
Owner

@kputnam kputnam commented Dec 17, 2015

This PR addresses incompatibilities with Rails and ActiveSupport caused by monkey patches in lib/ruby. Specifically issues and PRs #70, #67, #62, #63, #48 should (might?) be solved.

From what I can tell, this doesn't require any changes in client code unless someone was calling those monkey-patched methods directly. To make that work, clients will need to declare using Stupidedi::Refinements to bring the extension methods into scope.

Before merging this, I would like this to be reviewed and tested by someone other than myself. I've worked through several issues in the test suite, but I don't have a living application with which to test integration issues. If you have time, @irobayna, @alexkyllo, @johnstem, @serixscorpio, @limratana, @andrewthad, please try this branch out and let me know if you run into problems.

Some issues I ran into are documented here: https://gist.github.com/kputnam/c77e69264a2dc878ed8e

@kputnam
Copy link
Owner Author

kputnam commented Dec 17, 2015

I'm only realizing after seeing the build fail: this will exclude support for Ruby 2.0.0 and older, since refinements are kind of broken in Ruby 2.0.0 (they're only allowed at the file scope, I think) and don't exist prior to that.

Though it's almost three years old, does it pose a problem to existing users to require Ruby 2.1?

@irobayna
Copy link
Collaborator

@kputnam With Ruby 2.3.0 coming out in 7 days I believe it is ok to at least require Ruby 2.1

@irobayna irobayna closed this Dec 18, 2015
@kputnam
Copy link
Owner Author

kputnam commented Dec 18, 2015

Sounds good, @irobayna. I think you closed this PR by mistake :-)

@kputnam kputnam reopened this Dec 18, 2015
@irobayna
Copy link
Collaborator

@kputnam good catch! sorry, did not meant to close.
Should the version number be changed as well? Noticed you did not

@kputnam
Copy link
Owner Author

kputnam commented Dec 18, 2015

Which version number? I changed the Ruby versions in .travis.yml configuration but I've probably forgotten where it's referenced in documentation. Once this is merged, do you think it's a good time to also bump the version on stupidedi?

@irobayna
Copy link
Collaborator

Yes, I meant to say to bump stupedi version

I was getting some errors on lib\ruby\enumerator while running on Rails v4.2.4

  • NameError - undefined local variable or method tail' for [0, 0, 0]:Array:`
  • NameError - undefined local variable or method tail' for [0]:Array:`

By adding
using Stupidedi::Refinements within enumerator solves the issue.

module Enumerable

    using Stupidedi::Refinements
...

@irobayna
Copy link
Collaborator

With the line above I am able to run successfully using Rails parsing several large files (11,280 segments) and the rspec tests.

FYI rake spec does run successfully on:

  • ruby 2.2.4
  • ruby 2.3.0-preview2

@kputnam
Copy link
Owner Author

kputnam commented Dec 19, 2015

Thanks for the report Isi. I think I see the likely cause of the problem you mentioned, and I'm glad you caught that. The issue is I don't think you can refine modules only classes. So I still had to monkey-patch Enumerable the old way in order to define count and sum.

But I'll check again, I think those methods are never used. Or at least not used in a way that requires them to be defined as a monkey patch. I'll bet people call sum or count on all kinds of things in Rails and wouldn't be aware that the definition accidentally came from stupidedi :-)

@irobayna
Copy link
Collaborator

@kputnam way better, works as expected out of the box

@johnstem
Copy link

@kputnam This branch seems to have fixed my Rails issues as well. Nicely done! Thank you very much for the update!

@irobayna
Copy link
Collaborator

@kputnam let me know if you need anything to get this merge

@kputnam
Copy link
Owner Author

kputnam commented Jan 5, 2016

Unless anyone objects, I'll merge this on Friday.

kputnam added a commit that referenced this pull request Jan 9, 2016
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.

None yet

3 participants