I'd like to add a method to the String class for a gem I'm making. Obviously, I could just open it up and pop the method in, but I'd like to know what is the state-of-the-art for adding a method. Do I get anything better from putting the method in a module and then opening up the class and including it? I can't see why but I'd like to know.
I don't wish to use a new class that inherits from String because I understand that there's a gotcha with String due to something done for performance reasons (though I can't remember the exact details I remember the warning!), and there's obvious good sides to the method being directly in String that far outweigh the inheritance angle.
I think there's nothing wrong with monkeypatching (as long as you're
adding methods, not overriding them).
If you want to be on the extra-safe side, you could just create a
module with the method and have the users of your library actually
include it in String. There should be noticeable performance
difference between the two ways.
I don't have a problem with re-opening classes either. It's a tool in the Ruby tool box so why not use it. Especially when, as you pointed out, inheritance sometimes doesn't do what you'd expect. Some Ruby base classes don't call initialize which can be a problem.
Personally when I monkey patch I put my new methods in a module and then include them in the target class.
module MyStringExtensions
def foo
"bar"
end
end
class String
include MyStringExtensions
end
This has three benefits;
1. Reuse. You can include the same monkey patch in different classes. This almost never happens
2. Visibility. If you ask for the ancestors of the base class you will see your module in the list which will give users a clue why the class isn't behaving in a standard way and a pointer to where they should start looking.
3. Targeted extension. Using extend you can apply your monkey to specific objects rather than every object of that type in the system. Though, as @Bartosz pointed out, this may cause performance degradation's.
a = "asdfghj" # => "asdfghj"
b = "2345678" # => "2345678"
b.extend MyStringExtensions
a.foo # NoMethodError: undefined method `foo' for "asdfghj":String
b.foo # => "bar"
Please do not monkey patch core libraries in gems, unless the *purpose* of
your library is to add functionality to core, eg a library of extensions
like activesupport. Yes, this goes for additions as well as overrides. I
cannot tell you how many times I've seen a "harmless addition" mess things
up in confusing and hard to debug ways.
What method do you want to add to strings?
···
--
Avdi Grimm
On Jun 19, 2012 1:35 PM, "Iain Barnett" <iainspeed@gmail.com> wrote:
Hi all,
I'd like to add a method to the String class for a gem I'm making.
Obviously, I could just open it up and pop the method in, but I'd like to
know what is the state-of-the-art for adding a method. Do I get anything
better from putting the method in a module and then opening up the class
and including it? I can't see why but I'd like to know.
I don't wish to use a new class that inherits from String because I
understand that there's a gotcha with String due to something done for
performance reasons (though I can't remember the exact details I remember
the warning!), and there's obvious good sides to the method being directly
in String that far outweigh the inheritance angle.
Thanks for the reply. I'll relax a bit now in the knowledge it's not just me. But then maybe it's just us two!
Regards,
Iain
···
On 19 Jun 2012, at 19:01, Bartosz Dziewoński wrote:
I think there's nothing wrong with monkeypatching (as long as you're
adding methods, not overriding them).
If you want to be on the extra-safe side, you could just create a
module with the method and have the users of your library actually
include it in String. There should be noticeable performance
difference between the two ways.
2. Visibility. If you ask for the ancestors of the base class you will see your module in the list which will give users a clue why the class isn't behaving in a standard way and a pointer to where they should start looking.
3. Targeted extension. Using extend you can apply your monkey to specific objects rather than every object of that type in the system. Though, as @Bartosz pointed out, this may cause performance degradation's.
a = "asdfghj" # => "asdfghj"
b = "2345678" # => "2345678"
b.extend MyStringExtensions
a.foo # NoMethodError: undefined method `foo' for "asdfghj":String
b.foo # => "bar"
I just learnt about this yesterday, and though it's not applicable for the situation I've got at the moment, it's good to know, and this is the first code example of it I've seen too.
<rant>
While I understand it, I really disagree with this sentiment. Sure monkey patching can cause problems, but so can a lot of things. With great power comes great responsibility.
I really don't like this prevailing view that we should dumb down the language to the level of the least skilled developers. Banning unless/else and 'and/or', predicates returning true/false, inheritance is bad, etc. This is the wrong approach.
Instead we should be raising the skills of the developers to enable them to use the language to it's fullest.
</rant>
I think the OP's question has shown the right approach. He understands the dangers so, in an effort to improve his skills, he has asked the question, how can he use this technique in a way that will cause the least amount of problems. We should be encouraging this.
Henry
···
On 20/06/2012, at 12:23 PM, Avdi Grimm wrote:
Please do not monkey patch core libraries in gems, unless the *purpose* of your library is to add functionality to core, eg a library of extensions like activesupport. Yes, this goes for additions as well as overrides. I cannot tell you how many times I've seen a "harmless addition" mess things up in confusing and hard to debug ways.
Is there a way to limit the scope of an override or addition
to the namespace of the gem?
Here an example method that I use from time to time:
class String
def integer?
!!(self =~ /\A[+-]?[0-9]+\Z/)
end
end
Regards,
Marcus
···
Am 20.06.2012 02:23, schrieb Avdi Grimm:
Please do not monkey patch core libraries in gems, unless the *purpose*
of your library is to add functionality to core, eg a library of
extensions like activesupport. Yes, this goes for additions as well as
overrides. I cannot tell you how many times I've seen a "harmless
addition" mess things up in confusing and hard to debug ways.
And I answered the OP's question: he can cause the least amount of problems
by confining monkey-patching to his own apps, not in publicly distributed
gems. Or, if putting it in a gem, making it part of the publicly advertised
feature set of the gem, not a hidden surprise.
···
On Tue, Jun 19, 2012 at 9:51 PM, Henry Maddocks <hmaddocks@me.com> wrote:
I think the OP's question has shown the right approach. He understands the
dangers so, in an effort to improve his skills, he has asked the question,
how can he use this technique in a way that will cause the least amount of
problems. We should be encouraging this.
Please do not monkey patch core libraries in gems, unless the *purpose* of your library is to add functionality to core, eg a library of extensions like activesupport. Yes, this goes for additions as well as overrides. I cannot tell you how many times I've seen a "harmless addition" mess things up in confusing and hard to debug ways.
Right, one should be very careful when messing with core classes.
<rant>
While I understand it, I really disagree with this sentiment. Sure monkey patching can cause problems, but so can a lot of things. With great power comes great responsibility.
I really don't like this prevailing view that we should dumb down the language to the level of the least skilled developers. Banning unless/else and 'and/or', predicates returning true/false, inheritance is bad, etc. This is the wrong approach.
Instead we should be raising the skills of the developers to enable them to use the language to it's fullest.
</rant>
I think the OP's question has shown the right approach. He understands the dangers so, in an effort to improve his skills, he has asked the question, how can he use this technique in a way that will cause the least amount of problems. We should be encouraging this.
I disagree: since OP only asked our support for applying a specific
technique but did not disclose any details about what functionality he
wants to add and why we cannot even judge whether the approach was a
good or bad fit for the problem he is trying to solve. Could be that
a simple method which receives a String argument was sufficient etc.
Ian, please disclose a bit more detail about what problem you are
trying to solve.
Kind regards
robert
···
On Wed, Jun 20, 2012 at 3:51 AM, Henry Maddocks <hmaddocks@me.com> wrote:
Ok, sorry to be the cause of confusion and/or consternation!
Let me preface this by saying that I do understand the dangers (I hope), but that sometimes it is still appropriate so I wished to find out what the 2012 view point is on the implementation. I take the view that it's hard to ask a stupid question, and it's good to revisit things that you feel are basics or "done" to improve or perhaps change (or just find out you're still right! Things change, and I don't know everything (/much). Having said that, I can understand why anyone would give a strong "non!" to the idea without any specifics. I hope that was diplomatic enough
I spliced together two libraries that deal with Postgres' HStore document format (a No SQL field, essentially) into a new one as an update to a Sequel extension. I got it to pass the specs and do what I wanted, put in the pull request, and the maintainer (among other things) warned me that he'd prefer it if the core classes weren't opened up.
This prompted the question, though I realise I can do this other ways I'd also like to know the best way to do it this way - I was more interested in the general point than the specific solution. The decision to monkey patch was actually in one of the libraries I took from, I took their ball and ran with it until the specs passed and then put in the pull request. If it had been accepted then I'd have probably worked on it some more.
If you're still interested, the code in question is at https://github.com/yb66/sequel/blob/master/lib/sequel/extensions/pg_hstore.rb. The method(s) would be a Hash#to_hstore and a String#from_hstore. When I get a chance I'm going to make it into its own gem (on the advice of the maintainer) and I'll consider whether it's a good idea to continue with the monkey patches.
I my world String#from_hstore would be a class method at least. I
would also not cache @result in the String because that will prompt
all sorts of inconsistency issues when the String is changed.
Actually I'd rather have method HStore.from_string(str) and
HStore#to_string or maybe even HStore.from_db_string(str) and
HStore#to_db_string.
I am not sure I understand yet why you have three types involved:
String, Hash and HStore. You certainly need String and HStore. But I
would consider conversion to or from Hash just as a convenience
feature of HStore i.e. to easily create one. That would make changing
Hash superfluous. And all conversion methods could go into HStore as
explained above.
Btw, you can turn
token_pairs = token_pairs.map { |k,v|
[k,v].map { |t|
case t
into
token_pairs = token_pairs.map { |a|
a.map { |t|
case t
and save an Array creation.
Cheers
robert
···
On Thu, Jun 21, 2012 at 2:33 PM, Iain Barnett <iainspeed@gmail.com> wrote:
If you're still interested, the code in question is at https://github.com/yb66/sequel/blob/master/lib/sequel/extensions/pg_hstore.rb\. The method(s) would be a Hash#to_hstore and a String#from_hstore. When I get a chance I'm going to make it into its own gem (on the advice of the maintainer) and I'll consider whether it's a good idea to continue with the monkey patches.
On Thu, Jun 21, 2012 at 2:33 PM, Iain Barnett <iainspeed@gmail.com> wrote:
If you're still interested, the code in question is at https://github.com/yb66/sequel/blob/master/lib/sequel/extensions/pg_hstore.rb\. The method(s) would be a Hash#to_hstore and a String#from_hstore. When I get a chance I'm going to make it into its own gem (on the advice of the maintainer) and I'll consider whether it's a good idea to continue with the monkey patches.
I my world String#from_hstore would be a class method at least. I
would also not cache @result in the String because that will prompt
all sorts of inconsistency issues when the String is changed.
Actually I'd rather have method HStore.from_string(str) and
HStore#to_string or maybe even HStore.from_db_string(str) and
HStore#to_db_string.
I am not sure I understand yet why you have three types involved:
String, Hash and HStore. You certainly need String and HStore. But I
would consider conversion to or from Hash just as a convenience
feature of HStore i.e. to easily create one. That would make changing
Hash superfluous. And all conversion methods could go into HStore as
explained above.
Btw, you can turn
token_pairs = token_pairs.map { |k,v|
[k,v].map { |t|
case t
into
token_pairs = token_pairs.map { |a|
a.map { |t|
case t
Things change, and I don't know everything (/much). Having said that, I can understand why anyone would give a strong "non!" to the idea without any specifics. I hope that was diplomatic enough