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

Renamed the try method #48

Closed
wants to merge 282 commits into from
Closed

Renamed the try method #48

wants to merge 282 commits into from

Conversation

andrewthad
Copy link

No description provided.

@kputnam
Copy link
Owner

kputnam commented Aug 14, 2014

Thanks for the quick turnaround! Does this patch also rename present? to is_present?. I'm somewhat reluctant to merge this because the intention of those methods is to be compatible -- they were simply a shim to avoid bringing in a dependency on ActiveSupport for those two methods.

Can you paste the error and stack trace you had? I'd like to see what's involved in either making our try and present? methods behave like ActiveSupport, and I'd also like to consider just bringing ActiveSupport in as a dependency -- this comes up often enough that the cost of avoiding the dependency might be unjustified.

@andrewthad
Copy link
Author

I've also had another issue where I had to rename blank? to blankness?. I realized that the problem with try was that my older activesupport (and I'm using activesupport 2.3.18) defines try such that it cannot take a block as an argument. I'm pretty sure that your code does work for anyone on rails 3 or 4. I don't think you should change anything just to support rails 2.

@throttleup
Copy link

Any thoughts of using the ruby 2 refinement since there is so much monkey patching going on? I've been burned several times by the fact that Symbol has a call method

@kputnam
Copy link
Owner

kputnam commented Jul 24, 2015

@throttleup That sounds like a great suggestion. Before refinements were even proposed, I had mostly finished the library. If you or anyone else submitted a pull request for this, I would be happy to accept it.

@kputnam
Copy link
Owner

kputnam commented Dec 16, 2018

Closing this since it was solved with refinements in #72

@kputnam kputnam closed this Dec 16, 2018
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

7 participants