Constructor Parameters that aren't just slots
leonerd opened this issue · 8 comments
I've gone around my various CPAN code that's using Object::Pad (mostly the Device::Chip::* and Tickit::Widget::*) subclasses), looking for opportunities to get rid of BUILD blocks in favour of :param. So far I have managed to clean up about 30% of the named params to BUILD blocks, but that's all. The overall goal is to remove all the BUILD blocks in favour of something better composed by Object::Pad itself which can have knowledge of the entire union of key names everything expects, at which point it can complain about unrecognised arguments. Since BUILD blocks can arbitrarily inspect the passed args as a simple list (often assigning it into a hash), there is no way to tell what is or isn't a valid parameter name.
A lot of what is left falls into two broad categories: BUILD-time code that inspects args to initialise a slot, and BUILD-time code that just calls a mutator.
Dynamic slot initialisers
Typified by this somewhat-paraphrased example:
BUILD (%args) {
$self->push_choice( $_ ) for $args{choices}->@*;
}Here we're taking a choices constructor param whose value should be an ARRAYref, and calling a method on each element of it. It is tempting to suggest this be handled instead by an ADJUST block which can be annotated by :param to tell it what extra args to pass. An unanswered question is whether those args come in as a flat list in the given order, or as kvslice pairs.
ADJUST :param(choices) ($choices) {
$self->push_choice( $_ ) for $choices->@*;
}
ADJUST :param(choices) (%args) {
$self->push_choice( $_ ) for $args{choices}->@*;
}Calling mutator methods
Sometimes an object has a concept of a parameter that can be "set" or initialised by the constructor, but whose passed value isn't just a dumb store straight into the slot. For example, this paraphrased example stores an "interface" object, initialised by taking a parameter of the "interface type" name and constructing a helper object:
BUILD (%args) {
$self->set_iftype( $args{iftype} );
}
has $iface;
method set_iftype ($type) {
$iface = Iface->new_for_type($type);
}In this case, I wonder if it would be nice to support annotating :param(NAME) on a method, to request that the constructor call it from such a named argument.
has $iface;
method set_iftype :param ($type) {
...
}
# implies $self->set_iftype($args{iftype}) automatically in the constructor(If no name is specified then the parameter name defaults to using the name of the method it is attached to, after stripping a set_ prefix if present)
This latter would be usefully powerful, but also perhaps quite a lot of "magic at a distance" - the mere scattering of these :param annotations about the place causes the constructor to call them automatically. But do we want that?
(This issue is a continuation of an Object::Pad ticket - https://rt.cpan.org/Ticket/Display.html?id=137209 )
An unanswered question in all of the above is how to annotate these :param markers as being required vs. optional. When they are just applied to slots, the presence of a defaulting value can be used to notate that the parameter is optional. No such affordance exists for ADJUST or mutator-method params. Eg. in:
class Colour {
has $red :param;
has $green;
ADJUST :param(green) (%args) { $green = $args{green} }
has $blue;
method set_blue :param { $blue = shift }
}It is clear that red is a required parameter, because no defaulting expression. But how to specify this for either green or blue?
If perl syntax permitted ? in attribute names I might suggest we use :param vs :param? to notate it, but... :sadface:.
Maybe we'd add one of :required_param or :optional_param and have :param be "the other". Or maybe add both and don't permit :param alone on ADJUST or mutator-methods? But perhaps those names are long. :optparam vs :reqparam? The jury remains out...
Rather tired on my end, so need to reread this, but I paused at this:
BUILD (%args) {
$self->set_iftype( $args{iftype} );
}
has $iface;
method set_iftype ($type) {
$iface = Iface->new_for_type($type);
}
This is is manually writing a common pattern, which suggests that Corinna should handle it. Unfortunately, I think that properly needs types:
has $iface :isa(Iface) :coerce(Str => Iface->new_for_type($_));
(In the above, :coerce would take mutliple k/v pairs, making coercion somewhat easy)
Also, I was going with = undef; meaning "optional".
Looking at this example:
class Colour {
has $red :param;
has $green;
ADJUST :param(green) (%args) { $green = $args{green} }
has $blue;
method set_blue :param { $blue = shift }
}
We get this:
class Colour {
has $red :param = undef; # optional
has $green :param; # required
has $blue :param :writer = undef; # optional with mutator
}
Since that seems straightforward to me, I think I must be misunderstanding something. But as mentioned, I'm rather tired.
The iface example is an excellent suggestion for coërcion, yes.. I did consider that. I'm not sure I'm happy to put nontrivial amounts of perl code (or in fact, any code) inside attribute parens - I'd prefer that to live inside braces like
has $slot :coerce { CODE HERE };My Colour example was probably badly written because, as you point out, in that particular case all the behaviours are just dumb slot storage. I think rather than making up demo examples I'll link to some real code.
Lets take for example:
BUILD ( $text, %opts )
{
$_indent = $opts{indent} if defined $opts{indent};
@_chunks = $self->_build_chunks_for( $text );
}The indent can easily be handled with a :param, but not so the $text. Maybe this is another case for :coerce though? It would require us to have code-shaped coërcion blocks on nonscalar slots:
has @_chunks; :reader :param(text) :coerce { $self->_build_chunks_for($_) };Next up there's this example:
https://metacpan.org/dist/Tickit-Console/source/lib/Tickit/Console.pm#L81
BUILD ( %args )
{
my $on_line = $args{on_line};
...
$self->add(
$_tabbed = Tickit::Widget::Tabbed->new(
tab_position => "bottom",
tab_class => $args{tab_class} // "Tickit::Console::Tab",
),
expand => 1,
);
...
$_entry->set_on_enter( sub ( $entry, @ ) {
return unless $weakself;
my $line = $entry->text;
$entry->set_text( "" );
my $tab = $weakself->active_tab;
$tab->_on_line( $line ) or
$on_line->( $tab, $line );
} );
}The args tab_class and on_line aren't stored directly by this object. Instead they are passed down as parameters somehow related to construction of inner objects of this one. I really can't see how we would possibly do that in any way other than passing those in to an ADJUST block.
Another example here:
BUILD ( %args ) {
$self->tab_position(delete($args{tab_position}) || 'top');
}The only reason to take the %args here is to pass that directly down to a same-named mutator method. At first it seems simple enough to consider the :param notation on a mutator method to allow setting this:
method set_tab_position :param ($position) { ... }to request it be automatically invoked by the constructor. But then how do you specify that default? This shouldn't be a default value on the method's arg itself - we don't want to do
method set_tab_position :param ($position = "top") { ... }otherwise the default applies whenever invoking that method. We specifically only want that default applied by the constructor if no tab_position named argument was supplied. We begin to consider
method set_tab_position :optparam :paramdefault("top") ($position) { ... }which is starting to get really longwinded now. :/ Plus it involves putting nontrivial perl expressions inside attribute parens, which is always a terrible idea. Maybe we'd best not put params on methods, and just stick to having the ADJUST block call them explicitly:
ADJUST :optparam(tab_position) (%args) {
$self->set_tab_position($args{tab_position} // "top");
}Though there's still a two-times DRY failure with the word tab_position in this code, which I dislike.
Thoughts continue...
Regarding curly braces for :coerce, yes, those are much better.
Next, I see this:
has @_chunks; :reader :param(text) :coerce { $self->_build_chunks_for($_) };
What's the :reader? Does it return a list? Does it return an array reference? If we allow attributes for non-scalars, do we allow that for typeglobs? We need to be very careful in our semantics here. And what about this?
has @array :reader :writer :param;
In that, we're saying "you can pass this to the constructor", but how? If we pass that in as an array reference, why not just:
has $aref :reader :writer :param;
And for the @array example, if we can pass in an array reference, what is returned by the reader and are we passing a list of an array reference to set_array? I think it really needs to be array references all around, but we would expand them into the @array automatically. Unlike Raku, our arrays flatten automatically, so we have limited choices. I could easily be wrong in this, but we need to tread carefully here.
As for the rest of the stuff, I need to think on more of this later, but because it's complicating the design, I think most of this can be resolved easily for the MVP by simply specifying alternate constructors. But then there's this:
BUILD ( %args ) {
$self->tab_position(delete($args{tab_position}) || 'top');
}
I think what you want is this (I'm expanding this to include more code from the actual module).
has $_tab_class :param :reader(TAB_CLASS) = "Tickit::Widget::Tabbed::Tab";
has $_ribbon_class :param :reader(RIBBON_CLASS) = "Tickit::Widget::Tabbed::Ribbon";
has $_ribbon :reader;
has $_tab_position :param = 'top';
has @_child_window_geometry;
ADJUST {
$self->tab_position($_tab_position); # sets $_ribbon
$_ribbon->set_style( $self->get_style_pen("ribbon")->getattrs );
weaken($_tabbed)
}
About has @_chunks; :reader: I'd prefer to return an array (in the probably rare cases where you need a reader for such a slot). You want to copy your array slot anyway before returning it, lest the caller can change elements of your slots at will.
The same consideration applies for it really needs to be array references all around: Take has $aref :reader :param;: If you share that reference with the caller, they can change the contents of your array at any time, no :writer needed. In almost every case where my objects have FooRef (or object) attributes I'm using either BUILD or coerce to take a copy, to make sure that my object is the sole owner of its attributes. I consider has @_chunks :param a progress because it makes obvious that Corinna will take a (shallow) copy, even if the constructor needs to be passed an array reference because this is how Perl works.
Current thinking is that actually a lot of this can be done quite neatly by passing around all the remaining kvpairs in a single HASH reference, and allowing called blocks to delete from it. I'm undecided yet whether this should be the default behaviour of ADJUST blocks, or we should add a variant either named ADJUSTARGS or with an :args attribute or somesuch, but many of the original examples above can be done quite nicely this way:
has $_iface;
ADJUSTARGS ($args) {
$_iface = Iface->new_for_type( delete $args->{iftype} );
}Plus it lets the code neatly encode things like optional params:
ADJUSTARGS ($args) {
$self->set_iftype( delete $args->{iftype} ) if exists $args->{iftype};
}A small amount of DRY'ing here but it's all on one line so perhaps that's OK.
This also allows code to easily encode "default values" of such constructor params:
ADJUSTARGS ($args) {
$self->set_tab_position(delete $args->{tab_position} // "top");
}It also allows things like dynamic inspection or forwarding of entire sets of parameters, maybe named after some pattern. E.g.
ADJUSTARGS ($args) {
$_sslcontext = SSLContext->new(
# forward and delete all the "ssl_*" arguments
map { $_ => delete $args->{$_} } grep { m/^ssl_/ } keys %$args
);
}In fact the only thing it can't do neatly is specifying required params. Ideally, a check for required parameters would happen before any of the ADJUST blocks get invoked and cause a standard error message. At the moment there isn't such ability, so users would have to write
ADJUSTARGS ($args) {
exists $args->{iftype} or croak "Hey, so err, you forgot to pass iftype I guess";
...
}Perhaps that would be the purpose of a :params attribute on the ADJUSTARGS block?
ADJUSTARGS :params(iftype) ($args) {
# can delete $args->{iftype} in confidence now, knowing it definitely exists
}Though, while this easily encodes the requireness of the parameter, it in no way encodes a type constraint on it.
Plus all of this is dynamic in nature and not declarative - there's no way to introspect the class to ask it what parameter names it might want, whereas with :param declarations that would be possible.
@leonerd wrote:
Sometimes an object has a concept of a parameter that can be "set" or initialised by the constructor, but whose passed value isn't just a dumb store straight into the slot. For example, this paraphrased example stores an "interface" object, initialised by taking a parameter of the "interface type" name and constructing a helper object:
BUILD (%args) { $self->set_iftype( $args{iftype} ); } has $iface; method set_iftype ($type) { $iface = Iface->new_for_type($type); }
Currently, there are two ways we can handle this.
The first is to create two attributes. The first stores the actual type and the second gets initialized from the value of the first.
field $interface_type :param; # this is now required
field $interface { Interface->new($interface_type) };The second, of course, is the BUILD block as previously mentioned.
Closing this ticket now, because I think we should wait on post-MVP to see how the MVP plays out. I still don't see how ADJUSTARGS is significantly different from BUILD, aside from the fact that it would intercept the args before parsing passing them to the constructor, thus allowing us to delete args the constructor cannot handle. This makes it more or less the equivalent of BUILDARGS and we've agreed to drop that for now in favor of alternate constructors.