Perl-Apollo / Corinna

Corinna - Bring Modern OO to the Core of Perl
Artistic License 2.0
157 stars 19 forks source link

Attribute feedback #4

Closed Ovid closed 3 years ago

Ovid commented 4 years ago

Use this ticket to leave feedback on the Cor attributes proposal

leonerd commented 3 years ago

:constr sounds too close to :const. But maybe we can steal some C++ terminology and call it :ctor ? Isn't very obvious to a newcomer reading the code, though. How about :param ? As in "this takes a parameter (to the constructor)". Which makes more sense when you name it has $foo :param(bar);

damil commented 3 years ago

Excellent, I like :param

Ovid commented 3 years ago

We appear to be discussing a few different things, then. Let's make sure I'm clear on this:

  1. The slot variable, whose public name and constructor parameter name default to the variable name, minus the sigil
  2. The public name, if :reader and/or :writer are used
  3. The parameter name to the constructor

If we go with :param for saying "this is a parameter to the constructor," we also go further and say has $x :param(width); (Moo/se: init_arg => 'width') and that would be required in the constructor as width => $some_value. This would also set the public name to width, if a :reader or :writer is present, but the developer can confuse things further with :reader(other_name) if they so desire.

Is the above correct?

That reminds me, do we allow this?

has $x :reader :writer(width) :param(width);

This would be an attempt to overload the meaning of the width method to be both getter and setter. This is currently a widespread (mal)practice that I'm guilty of myself. Without multi-dispatch, it's not something to be encouraged, but detecting and handling this case might be hugely popular (or at least, throwing an exception on the above line might be hugely unpopular).

damil commented 3 years ago

@Ovid wrote

  1. The parameter name to the constructor

Rather : 3. The fact that this slot gets initialized through the constructor, and if so, through which named parameter

This would also set the public name to width

No, I think this is a bad idea because there would be too much semantics for one construct. The public name should definitely stay with :reader. By contrast, :param or :ctor should only specify what happens with the constructor. These two concepts should stay orthogonal to avoid confusion.

do we allow this :has $x :reader :writer(width) :param(width);

Having the same method name for reader and writer is a very common idiom in many OO languages. People may have good reasons to stick with that idiom, so there should be a way to do it in Corinna, even if it is not encouraged.

Ovid commented 3 years ago

@damil wrote:

No, I think this is a bad idea because there would be too much semantics for one construct. The public name should definitely stay with :reader. By contrast, :param or :ctor should only specify what happens with the constructor. These two concepts should stay orthogonal to avoid confusion.

I'm very sympathetic to that point of view. I have a number of thoughts on this and deleted all of them because I realized they're more appropriate for v2.

duncand commented 3 years ago

Personally I'm more of a purist of sorts who believes functions and procedures are very distinct concepts where the first is non-mutating and returns a value and the latter is possibly mutating and doesn't return a value (but it can mutate a parameter).

I believe it is best to have separate methods for reading and writing a slot, and not overloading a single method to do both. Or if the latter exists it should be optional and logically a wrapper for the other two; the two separate getter/setter should at least be available and ideally be the default.

duncand commented 3 years ago

I like the idea of param but feel we might be able to do better. The name param still seems somewhat ambiguous and doesn't specifically say "constructor" to me. What do people think about cparam? Short for "constructor parameter".

Grinnz commented 3 years ago

I have no clear preference between "different getter and setter" and "single accessor", I have used both for different types of projects, but I do have a preference that either be easily possible, and that any setter (including the combined accessor when called with a value) can be made chainable (return the invocant).

Ovid commented 3 years ago

I think some of these may need to be punted to V2. Also, I've had people tell me they won't use Corinna if mutators are chainable and others tell me they must be. I wonder what the best way of handling that is?

rabbiveesh commented 3 years ago

Is the class able to have colon annotations? You could have a 'fluent' or a 'chainable' attribute for the class which dictates the behaviour of mutators.

On Thu, Jun 17, 2021 at 4:10 PM Ovid @.***> wrote:

I think some of these may need to be punted to V2. Also, I've had people tell me they won't use Corinna if mutators are chainable and others tell me they must be. I wonder what the best way of handling that is?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/Ovid/Cor/issues/4#issuecomment-863226132, or unsubscribe https://github.com/notifications/unsubscribe-auth/AFURPKW45QAG7SK766OQVZ3TTHX6BANCNFSM4LRIHJKA .

leonerd commented 3 years ago

Is the class able to have colon annotations? You could have a 'fluent' or a 'chainable' attribute for the class which dictates the behaviour of mutators.

The individual mutators declare this; at least in Object::Pad:

https://metacpan.org/pod/Object::Pad#:writer,-:writer(NAME)

https://metacpan.org/pod/Object::Pad#:mutator,-:mutator(NAME)

I don't yet have an attribute to request a "takes 0 or 1 arguments and does totally different things depending on that" style of accessor, but I could easily add one if folks really demanded it. I tend to dislike that style though, personally.

rabbiveesh commented 3 years ago

Is the class able to have colon annotations? You could have a 'fluent' or a 'chainable' attribute for the class which dictates the behaviour of mutators.

The individual mutators declare this; at least in Object::Pad:

https://metacpan.org/pod/Object::Pad#:writer,-:writer(NAME)

  • default named set_SLOTNAME
  • returns $self

https://metacpan.org/pod/Object::Pad#:mutator,-:mutator(NAME)

  • default named SLOTNAME
  • yields lvalue so allows assignment into it

I would imagine that it makes more sense for this sort of thing to be class wide rather than set on each method. I think it enforces consistency in design. Or at least the class should be able to declare a default.

Although, putting it at the class level leaves up for grabs how the accessors from roles or inheritance would act. Who creates the accessors when a role is applied, the class or the role?

Ovid commented 3 years ago

I would imagine that it makes more sense for this sort of thing to be class wide rather than set on each method. I think it enforces consistency in design. Or at least the class should be able to declare a default.

Consistency of design is very important. However, if we have separate :writer and :mutator attributes, this causes two issues.

  1. Different slots can use different attributes (especially if you're consuming a role), leading to inconsistent behavior.
  2. Corinna now needs to validate that you haven't specified both :writer and :mutator on the same slot.

I've tried very hard to minimize any possible conflicts in slot attributes and now we're talking about creating one. The more we create, the more work Corinna has to do the guard against them and the more likely it is that developers are going to get it wrong.

leonerd commented 3 years ago

I'm not sure @haarg's question ages ago really got a proper answer to it, so here again more concretely:

Is this a compiletime syntax error?

class AClass {
  has $data :param;
  method message { say "AClass's data is $data"; }
}

class BClass isa AClass {
  has $data :param;
  method message { $self->next::method; say "BClass's data is $data"; }
}

If it is not a syntax error, then is this a runtime error?

my $obj = BClass->new( data => 123 );

If that doesn't raise an error, then what does this print?

$obj->message;
duncand commented 3 years ago

I think some of these may need to be punted to V2. Also, I've had people tell me they won't use Corinna if mutators are chainable and others tell me they must be. I wonder what the best way of handling that is?

I feel the best thing for V1 is to make mutators not return anything at all and expect to be called as a statement.

This is the simplest, the least surprising, and the most forwards-compatible.

Corinna V2 could possibly change things so that mutators return something, and it can be decided then what that is, but meanwhile a V1 that doesn't means no code will rely on using a return value and so no code written for V1 will break when used with V2.

I feel the best V1 feature for people wanting to set multiple slots at once after construction is to have a standard single system-defined assign or write or mutate whose parameter format is exactly the same as new or clone would take, a single list/hash of name-value pairs.

Any design pattern using chained setters can instead use named constructor or mutator parameters, its a single statement that looks like name-value pairs either way.

This new method could also return its innocent so it can be chained if one wishes.

Ovid commented 3 years ago

@leonerd: for v1, that should be a compile-time error as covered in section 3 of the constructor logic. In future versions, a compile time error would be lovely.

robrwo commented 3 years ago

I'm concerned that the defaults of

has $x

being private and not in the constructor are so different from the defaults in Moo/Moose/etc that this will lead to a lot of confusion, and that people working with legacy and Cor codebases will regularly make mistakes.

duncand commented 3 years ago

I'm concerned that the defaults of

has $x

being private and not in the constructor are so different from the defaults in Moo/Moose/etc that this will lead to a lot of confusion, and that people working with legacy and Cor codebases will regularly make mistakes.

Any mistakes resulting from this will only be ones that make the Perl code safer, rather than the opposite which would be the case if the defaults were public, so default private is a GOOD thing.

As for differing from prior experience with other modules, that is best addressed in documentation/tutorials/etc, which would make it clear up front that the defaults are private etc.

robrwo commented 3 years ago

Any mistakes resulting from this will only be ones that make the Perl code safer...

Private methods in general don't make code any "safer".

As for differing from prior experience with other modules, that is best addressed in documentation/tutorials/etc

No amount documentation or tutorials will keep even the most experienced developers from making mistakes, because it's using the same keyword as other OO systems have ad for the past 15+ years, but with a different semantics and syntax.

If a different keyword was used ("attr"?) then I'd have no problem with that.

duncand commented 3 years ago

@robrwo said:

Any mistakes resulting from this will only be ones that make the Perl code safer...

Private methods in general don't make code any "safer".

As for differing from prior experience with other modules, that is best addressed in documentation/tutorials/etc

No amount documentation or tutorials will keep even the most experienced developers from making mistakes, because it's using the same keyword as other OO systems have ad for the past 15+ years, but with a different semantics and syntax.

If a different keyword was used ("attr"?) then I'd have no problem with that.

OH! Well that's completely different.

What you said before totally came across to me that the problem you had was attribute declarations defaulting to private period, regardless of the keyword. But now its clear that the actual problem was using the same keyword with different behaviour.

In that case, I totally support using a different keyword to declare attributes. The keyword attr should work.

duncand commented 3 years ago

Private methods definitely do make code "safer" because it means the language is more strict and constraining on what it allows. Private methods make things safer like "use strict" makes things safer. It means the language itself prevents people from calling these methods from somewhere they're not supposed to call them.

duncand commented 3 years ago

Actually using the keyword "slot" may be better than "attr" because it matches the term all the documentation talks about, and its a full word rather than an abbreviation of a word. Unless Ovid has something against using documentation terms as keywords. I only see using "slot" as being a problem if a "slot" is actually some more abstract term that the declaration code doesn't exactly map to, and then it can be confusing.

Ovid commented 3 years ago

Private methods in general don't make code any "safer".

Private methods absolutely make code safer. The first time in DBIx::Class that you accidentally override a dbic method that you didn't realize you were overriding ... good luck tracking that down. I've had really fun debugging sessions with that. With private methods, that pain simply ceases to exist.

But what if the method isn't private? Many languages have learned the hard way that silently overriding methods is a bad thing because in large systems, devs don't know this is happening. So in Java, for example, you must provide an @Override annotation or you get a warning. You might be surprised to realize you were overriding something, but at least you'll realize it.

Regarding using slot or attr instead of has, I do take the point that the semantics are different from Moo/se. I don't have a strong objection to changing that, so long as there's general agreement.

robrwo commented 3 years ago

Private methods absolutely make code safer.

Ok, fair point that I'd forgotten about.

Regarding using slot or attr instead of has, I do take the point that the semantics are different from Moo/se. I don't have a strong objection to changing that, so long as there's general agreement.

I'm in favor of "attr" myself.

Also note a "has" extension could be written that converts Moo/se "has" into Cor "attr" or "slot" with minimal confusion.

abraxxa commented 3 years ago

To me the term 'slot' has no meaning when it comes to OO. Is there any language using that term for an object attribute?

haarg commented 3 years ago

Other languages call these fields, properties, member variables, instance variables. The "name" of them generally isn't used to define them though. Moose calls them attributes, but uses has to define them. Perl itself has fields.pm for this type of data.

attr implies that these are named "attributes". Perl already has a concept of attributes, which Corrina is making heavy use of, at least syntactically. slot is pretty meaningless. I'm not aware of any other language or system using the term "slot" for this. has fits reasonably with my and our, although it isn't the same form of word. "my" and "our" are possessive determiners. There aren't really any other appropriate words of this class ("his", "her", "its", "their") that could be used.

Of these options, I still prefer has, as I think it matches best with the existing language and doesn't conflict with anything in core. Corrina is going to have many concepts that overlap with but work differently from Moo/se. I don't think that should prevent those from being used.

I'd be open to other suggestions though.

robrwo commented 3 years ago

Of these options, I still prefer has, as I think it matches best with the existing language and doesn't conflict with anything in core. Corrina is going to have many concepts that overlap with but work differently from Moo/se. I don't think that should prevent those from being used.

If it was similar to Moo/se has, I'd be fine with it.

But after 10-15 years of using Moo/se, I write and read "has" a certain way. I think a significant change in the defaults will frustrate a lot of developers (especially people newer to Perl who may have had some exposure to Moo/se).

I'd rather a different keyword that makes it clear that this is a native Corinna attribute/property/slot/field/etc.

As for the keyword, I think "attr" (attribute) or "prop" (property) is best. "Attribute" is widely used in Perl OO terminology.

"field" is usually associated with forms (HTML::FormHandler or Form::Tiny) and sometimes databases.

I'm not a fan of "slot" since it's not widely used. I fear the term would start leaking into documentation, so you'd start seeing modules with POD listing "slots" instead of "attributes".

Grinnz commented 3 years ago

IMO, the fact that this property declarator will always declare properties with sigils is enough to differentiate it from existing usage of 'has', even if it shares the name.

jjn1056 commented 3 years ago

I'd encourage using anything other than 'has'. When I see 'has' I think 'Moose'. The Cor developers have decided to toss out 10+ years of Moose community practice in favor of something totally else. If that's the choice then place make sure there's nothing in Cor that makes me think its like Moose.

leonerd commented 3 years ago

I think I came up with the word slot early on in Object::Pad docs (right back in v0.01) just as a word that wasn't "attribute". I'm not overly attached to it as a term for documentation - happy to entertain alternatives.

I am with @haarg on avoiding the word "attribute" though - those :thing markers we make lots of use of, are already called "attributes" by Perl itself. We should pick a word other than that.

leonerd commented 3 years ago

Oops. I have just been reminded:

20:29 <haarg> LeoNerd: slot came from stevan's MOP iirc
20:30 <LeoNerd> Ooooh.. right.
20:30 <LeoNerd> Yes - that was mentioned at Amsterdam 2019
clscott commented 3 years ago

I think I came up with the word slot early on in Object::Pad docs (right back in v0.01) just as a word that wasn't "attribute". I'm not overly attached to it as a term for documentation - happy to entertain alternatives.

I am with @haarg on avoiding the word "attribute" though - those :thing markers we make lots of use of, are already called "attributes" by Perl itself. We should pick a word other than that.

Arguments against has and for slot here which a have mostly been covered above: https://github.com/Ovid/Cor/issues/5#issuecomment-612882068

My main argument though is perl slot is going to a perfect google search term to lead directly to the canonical docs and associated tutorials and will not interesect with Moose,Moo, Mouse or anything else requiring the learner to have to tease Cor from the others. Additionally python, javascript, R, Dylan, “Common Lisp Object System” and Stevan Little's MOP use 'slot' to refer to items of data stored in an object instance. Somewhat ironically python also has at least one other way to set up instance data members.

I agree that anything approaching attribute as a name is right out and was a poor choice in Moose nomenclature due to perl already having a feature called attributes.

Additionally it would be nice if a similar noun could be chosen for declaration of class data IF class data is going to be a thing. Possibly a longer name to disuade devs from using it? class_slot? component? This way the new system will have nouns followed by their name and defintion starting with class.

slot++

duncand commented 3 years ago

I think I came up with the word slot early on in Object::Pad docs (right back in v0.01) just as a word that wasn't "attribute". I'm not overly attached to it as a term for documentation - happy to entertain alternatives.

I am with @haarg on avoiding the word "attribute" though - those :thing markers we make lots of use of, are already called "attributes" by Perl itself. We should pick a word other than that.

Speaking about programming languages generically, I prefer/use the term attribute for a component of a heterogeneous structure characterized by a set of name-value pairs such as a record or database tuple or struct or object.

However Perl already uses attribute heavily for those :thing markers which are becoming even more heavily used in newer Perls and so Corinna shouldn't use that for its slots because the ship has sailed.

Ovid commented 3 years ago

This is resolved for the MVP. Further issues should be new tickets.