On Sep 28, 2008, at 01:23, Eric Wilhelm wrote:
Right. So that's part of why I like many small patches. Something
like that makes the test pass, but it usually points to a bigger
problem.
My patch was small. It did only two things:
* Add support for params to add_property()
* Add support for coderef defaults to add_property().
The latter could easily be rolled back if you had wanted them
separately. The scoping of `no strict 'refs'` was a simple refactor I
could do while I was in there.
I think it needs to just go with "no strict 'refs'" in each of those
blocks (with a comment mentioning "class method"!) and then we'll
triage the symbolic ref issue in another go.
Well, it appears that they only ever get called as $class->quiet()
and $class->get_options(), both of which are our own fault.
Right. I looked briefly at trying to fix that, but didn't have any
luck. It seems that by allowing the `no strict 'refs'` bit, the
properly can continue to be returned after it has been set. By not
using it, and constructing a hashref each time, of course it is not
set after the first time.
As for the rest of the world...
We'll have to see what we discover during the alpha run and perhaps
we'll find some stale copy+pasted methods there, but for now (I
think perhaps a release or two) I think the thing to do is to carp()
and return undef. In the long run, it should either croak() or do
something useful.
Yeah. I doubt that there will be real-world problems. add_property()
was undocumented, and the class interface to it was even less clear (I
had no idea until tests started failing with my refactor).
That is, If I'm reading this correctly, anybody using $class-
>foo('bar') as a setter is not going to get 'bar' back in $class-
>new->foo() anyway (because it went in $Module::Build{foo} = 'bar' -
which is afaict not referenced during object construction), so it
seems safe to "break this unexpected behavior".
That's what I thought, too. But the `no strict 'refs'` bit allows it
to actually be returned~
% perl -MModule::Build -le 'Module::Build->installdirs("foo"); print
Module::Build->installdirs'
foo
Crazy hack, eh? Ken, was there a good reason for this?
As for the rest of your patch David, would you mind porting it
forward to fit this refactor? I think the API you'll want is
basically like: _make_accessor($property, %opts).
Yes, I'll do that, but I'm less inclined to contribute in the future
if you force me to rewrite my patches because you discover, thanks to
my patch, that you don't like the current code, so you change it and
force me to rewrite. Please, if you're going to do that, wait until my
code is committed so that there's less work to be done overall.
Best,
David
