Beefy Boxes and Bandwidth Generously Provided by pair Networks
No such thing as a small change
 
PerlMonks  

Class::MethodMaker, workplace politics, and patches

by dragonchild (Archbishop)
on Feb 11, 2005 at 17:33 UTC ( [id://430184]=perlquestion: print w/replies, xml ) Need Help??

dragonchild has asked for the wisdom of the Perl Monks concerning the following question:

I just started using Class::MethodMaker this week. I'm writing a new distro (to deal with the OpenURL standard) and my lead asked me to write it using C::MM, which I thought was cool. It's what's installed on all the servers where I'm working* and I've heard good things about it.

So, I set up a few classes and almost immediately run into a bug with C::MM 2.05. I patched my local version and continued on my way. No biggie - this happens all the time. Patching it was a little weird cause I've never worked with AutoSplit before, but that's ok - it never hurts to learn something new.

Now, here's where the politics of opensource and the politics of my employer start intersecting. I can't ask FLUFFY to speed up fixing my bug and getting it into a new release2. I also can't keep holding off the upgrade from C::MM 1.12 to 2.x. And, they won't install my patched version because it's not the official version.

To top it all off, I have an enhancement I really want added to make my coding life a lot easier. I mean, if that's not the whole point, why use C::MM in the first place? But, I wanted to run it past y'all first before bugging FLUFFY about it. I want to have an attribute that looks something like:

use Class::MethodMaker [ new => 'new', scalar => [ { -type => 'Some::Class', -default_ctor => 'new' }, 'some_class', ], ];

Now, I understand that whenever you do a get on some_class(), you'll get back a new Some::Class object, and that's the desired behavior. But, I want to be able to do

$object->some_class_reset(); ok( !$object->some_class_isset(), "I just reset the damn thing, so it shouldn't be set yet!", );

Except, C::MM has an optimization that says if a default/default_ctor is defined, the attribute is always considered to be set, even if it has just been reset. Is there a way around this without patching C::MM::scalar? And, if there isn't, does anyone have any advice for me on patching it? FLUFFY's code is downright scary. Very cool, but scary all the same.

And then, does anyone have any advice for me re: the office politics vs. the opensource politics? Does anyone have a good mailing list/forum for C::MM that has had more than 30 messages in the last 2 years?

  1. It's a real PITA to get stuff installed at this place. I can't even get Text::xSV installed as a bugfix over Text::CSV. There's over a hundred common servers and a Sun workstation at almost every employee's desk. (I'm a contractor.) Perl isn't the major management focus, so the only guy with both the rights and the know-how to install Perl modules is the network admin. In other words, I would have to figure out how to get him to spend part of his 60-hour workweek (in which he does 100 hours of work) to install my module. Yeah, riiiiight.
  2. I got pissed off when someone did that with me for PDF::Template, especially cause they were asking for 80 hours of unpaid work to be done in a month.

Being right, does not endow the right to be rude; politeness costs nothing.
Being unknowing, is not the same as being stupid.
Expressing a contrary opinion, whether to the individual or the group, is more often a sign of deeper thought than of cantankerous belligerence.
Do not mistake your goals as the only goals; your opinion as the only opinion; your confidence as correctness. Saying you know better is not the same as explaining you know better.

Replies are listed 'Best First'.
Re: Class::MethodMaker, workplace politics, and patches
by BrowserUk (Patriarch) on Feb 11, 2005 at 18:13 UTC

    You ain't gonna like the suggestion but...

    You are using a dynamic language, apply the patches dynamically.

    That is, c&p the smallest replaceable piece of code that the patch effects, into your code. Apply the patch to the c&p and dynamically replace the sub/method at runtime.

    Make that dynamic patch dependant upon the version number of the module you are patching.

    Add a testcase and/or in-line test for that will detect if the pre-patched code has been fixed in a later version and if it has, disable the code and issue a warning to the logfile noting that the dynamic patch code can be removed.

  • Document the whole thing very thoroughly, both in the source, and in a separate document that explains:
    1. Why the patch was required--technical reasons.
    2. Why it couldn't be applied in the normal way--polital reasons.
    3. An copy of the email sent to the author requesting the change/addition. Plus any follow-up.

    Get on with the job using the patches and give the document to your supervisor--once you have a sufficient body of work & tests to demonstrate the benefits.

    You should probably include an example of what extra work would be required or what benefits would be lost were the patch not applied.


    Examine what is said, not who speaks.
    Silence betokens consent.
    Love the truth but pardon error.

      Seconded. Depending on whether the next Algorithm::Dependency includes a patch from me something very similar to what you proposed will be making it into the next OpenInteract2 beta. Mine was very straightforward though (just adding a new method for some new functionality). But I don't think it's that risky since it's temporary -- once the new A::D comes out (and assuming it has my patch) I'll just bump up the version in Makefile/Build.PL and remove the workaround.

      Chris
      M-x auto-bs-mode

        It's a useful technique. We used to send out binary patches that attached themselves to the end of the afflicted executable and patched themselves into the load table.

        When the executable loaded, they checked the embedded version in the 3rd party .dll that contained the bug we were working around, and if it was broken version, patched the dynamic link tables to redirect the broken API to the substitute that we had attached to the end.

        When the 3rd party library was updated, the next time the executable ran, it unhooked itself from the loadtable, and became a redundant few dozen extra bytes on the end of the executable, and the program continued, now using the (hopefully fixed) API.

        Of course, you wouldn't get away with this these days because every virus scanner within a million miles would scream bloody blue murder.

        Such is progress.

        That said. C::MM is kind of unusual as the code that needs to be patched never actually exists on disk anywhere. It jusy comes into being in memory at compile time. Kind of tricky to C&P.


        Examine what is said, not who speaks.
        Silence betokens consent.
        Love the truth but pardon error.
Re: Class::MethodMaker, workplace politics, and patches (A dynamic patch)
by BrowserUk (Patriarch) on Feb 11, 2005 at 19:59 UTC

    Yeah,yeah. I know. I suck!

    #! perl -slw use strict; use Data::Dumper; package Foo; sub new { bless {}, $_[ 0 ] } package MyClass; use Class::MethodMaker [ scalar => [ { -type => 'Foo', -default_ctor => 'new' }, 'foo', ], ]; sub new { my $class = shift; bless {}, $class; } if( @ARGV and $Class::MethodMaker::VERSION eq '2.05' ) { no warnings 'redefine'; *MyClass::foo_isset = sub : method { package Class::MethodMaker::scalar; exists( $_[0]{foo} ); } } package main; my $m = MyClass->new; print $m->foo; $m->foo_reset; print $m->foo_isset ? 'foo is set' : 'foo is not set'; __END__ [19:57:31.35] P:\test>430184 Foo=HASH(0x225000) foo is set [19:57:33.20] P:\test>430184 patchit Foo=HASH(0x225000) foo is not set

    Examine what is said, not who speaks.
    Silence betokens consent.
    Love the truth but pardon error.
      Thank you! This is exactly what I was looking for! :-)

      Being right, does not endow the right to be rude; politeness costs nothing.
      Being unknowing, is not the same as being stupid.
      Expressing a contrary opinion, whether to the individual or the group, is more often a sign of deeper thought than of cantankerous belligerence.
      Do not mistake your goals as the only goals; your opinion as the only opinion; your confidence as correctness. Saying you know better is not the same as explaining you know better.

      Morning,

      Sorry to be late to the party. BrowserUk, your answer is quite right; but you can make it slightly easier on yourself by simply defining foo_isset in the class before Class::MethodMaker(->import) is called. It will never overwrite an existing method. Since the example does a use, that would need to be in a BEGIN{} ahead of the use.

      Mx.
Re: Class::MethodMaker, workplace politics, and patches
by BrowserUk (Patriarch) on Feb 11, 2005 at 18:51 UTC

    I'm probably misunderstanding your post, or the POD, but doesn't this snippet from the C::MM pod indicate that when you reset something, the is_set() method return false? And isn't that the behaviour you are after?

    my $m = MyClass->new; my $a, $x; $a = $m->a; # *undef* $x = $m->a_isset; # false $a = $m->a(1); # 1 $m->a(3); $x = $m->a_isset; # true $a = $m->a; # 3 $a = $m->a(5); # 5; $m->a_reset; $x = $m->a_isset; # false

    Examine what is said, not who speaks.
    Silence betokens consent.
    Love the truth but pardon error.
      You missed the part about it having a default_ctor. Use of -default_ctor or -default changes the behavior of *_isset(). I'm not arguing the design decision, but I want to be able to turn off the optimization.

      Here's a testcase to demonstrate what I'm talking about.

      use strict; use Test::More 'no_plan'; package Bar; sub new { bless {} } package Foo; use Class::MethodMaker [ new => 'new', scalar => [ { -type => 'Bar', -default_ctor => 'new' }, 'bar', ], ]; package main; my $foo = Foo->new; isa_ok( $foo, 'Foo' ); my $bar = $foo->bar; isa_ok( $bar, 'Bar' ); $foo->bar_reset; ok( !$foo->bar_isset, "No get() after reset(), so why is it set()?" ); ----------------- ok 1 - The object isa Foo ok 2 - The object isa Bar not ok 3 - No get() after reset(), so why is it set()? 1..3

      Being right, does not endow the right to be rude; politeness costs nothing.
      Being unknowing, is not the same as being stupid.
      Expressing a contrary opinion, whether to the individual or the group, is more often a sign of deeper thought than of cantankerous belligerence.
      Do not mistake your goals as the only goals; your opinion as the only opinion; your confidence as correctness. Saying you know better is not the same as explaining you know better.

        I have to say, that having read a bit more about C::MM and how it works, I disagree with teh change you are making.

        You question (in the tests) is:

      • "If I've reset() an attribute and not yet done a get() or set(), why does it test as isset()?"

        And I would say, because it has a default.

        Therefore if you did do a get(), a value would be returned, and that value would be a real value--even if it was undef because the default was set to be undef.

        So, the envisioned use of _isset is to allow the caller to determine if they will get a legitimate value when they call get()--even if when they do call it they get undef.

        But you appear to be trying to use isset() as if it were isreset(), or possibly _isdefault().

        Which may be a desirable test to be able to perform for some reason other than testing that _reset() works--which is C::MM's test suite responsibility, not yours--but if it is, then it has different semantics to _isset and should probably be a different method.


        Examine what is said, not who speaks.
        Silence betokens consent.
        Love the truth but pardon error.
Re: Class::MethodMaker, workplace politics, and patches
by fluffy (Scribe) on Mar 13, 2005 at 12:35 UTC
    Dragonchild,

    Don't be reticent to email me. As you've seen, I can be slow to reply, about which I apologize and will try to do better, but I certainly don't object to receiving the email. I rarely get to look at PerlMonks, which sucks because it's cool, so email is good.

    As BrowserUk suggests, I'm very wary of what you're proposing, since it breaks the seemlessness of the -default, which is supposed to look like it's always set. I appreciate that you're asking for an override option, but the idea of adding another special case to what is already some pretty special-cased code (mostly for v1 compatibility) is scary. Perhaps you can explain what you're trying to achieve under the hood

    Mx.

Log In?
Username:
Password:

What's my password?
Create A New User
Domain Nodelet?
Node Status?
node history
Node Type: perlquestion [id://430184]
Approved by Tanktalus
help
Chatterbox?
and the web crawler heard nothing...

How do I use this?Last hourOther CB clients
Other Users?
Others cooling their heels in the Monastery: (4)
As of 2024-04-20 16:07 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    No recent polls found