Perl-Apollo / Corinna

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

RFC Feedback #26

Closed Ovid closed 3 years ago

Ovid commented 3 years ago

Please leave feedback on the Corinna RFC here.

Ovid commented 3 years ago

@merrilymeredith wrote:

Could a trailing comma be permitted on the role list following does, or was there already a decision against it?

D'oh! Of course. I just updated the grammars to include that.

leonerd commented 3 years ago

Small question though: How is the parser going to be able to tell? In particular in the following

class C does eats, shoots, leaves {
   ...
}

Does that mean this class performs three named roles, or performs two named roles and also has the leaves declaration after a trailing comma?

Without some other delimiter here it is impossible to tell. If we want to permit trailing commas then we will have to surround the items in parens or somesuch:

class A does (eats, shoots, leaves) {
   ...
}

class B does (eats, shoots,) leaves {
   ...
}
duncand commented 3 years ago

Without some other delimiter here it is impossible to tell. If we want to permit trailing commas then we will have to surround the items in parens or somesuch:

@leonerd We thought alike.

nael-binary commented 3 years ago

Based on some usage trials, and experimenting with using such approach, by using Object::Pad since it's the closest one to it so far as mentioned. One thing to point out would be about the ADJUST block, which is similar to the BUILD block in Object::Pad. Maybe starting it with a broken example can make it clearer :)

use Object::Pad;

class Parent {
    has $common = {};
    has $x :param reader writer;

    BUILD (%args) {
        $common->{points}++;
    }
}

class Child extends Parent {
    has $y :param reader;

    BUILD (%args) {
        $self->set_x($y);
        # Or even modify $y value before setting it, like this
        $y = $args{y} * $self->common->{points};
    }
}

my $p = Child->new(y => 2);
# Required parameter 'x' is missing for Child constructor at ...

Maybe there should be some ability to unpack and handle child values first, and have some control over what's passed back to the parent? as changes to parent classes could lead to broken subclasses. (removing :param from $x in Parent class would make it work, more about :param in below example) I have added that $common to show that maybe having has is enough and there is no need for all current modifier types to be defined, especially since we are hoping to make this part of Perl core, so its more like keeping the flavour of Perl present there too, and hopefully accepted. But that is just my view, and Im fine with other views :) also need to note that this common mixed with :param approach at the moment may lead to things like:

use Object::Pad;
class MandatoryDefault {
    has $x : param;

    BUILD  (%args) {
        $x = $args{x} // {}; 
    }
}
MandatoryDefault->new;
# Required parameter 'x' is missing for MandatoryDefault constructor at ...

and if you added that default to the has definition like has $x : param = {}; it will work however you'd maybe accidentally changed you slot to become common type instead of just setting the default for this particular object.

kernigh commented 3 years ago

Replying to @nael-binary, Object::Pad has BUILDARGS. I can avoid, "Required parameter 'x' is missing...", by defining BUILDARGS to insert x => $args{y} before the parent class needs x.

use Object::Pad;

class Parent {
  has $x: param reader;
}

class Child isa Parent {
  has $y: param reader;

  sub BUILDARGS($class, %args) {
    $args{x} = $args{y};
    $class->SUPER::BUILDARGS(%args)
  }
}

my $p = Child->new(y => 2);
printf "x = %s, y = %s\n", $p->x, $p->y;
# OUTPUT: x = 2, y = 2

Corinna (as I understand it) wouldn't have BUILDARGS, so the above example for Object::Pad would not work in Corinna. If I don't use BUILDARGS, I might try to redefine Child->new. This almost works in Object::Pad,

class Child isa Parent {
  has $y: param reader;

  my $new;
  BEGIN { $new = \&Child::new }

  sub new($class, %args) {
    $args{x} = $args{y};
    $class->$new(%args);
  }
}

It broke after I made class Grandchild isa Child { ... }, because Grandchild->new died with, "'x' is missing", and ignored my code in Child->new to fill in x.


Replying to @tm604, who asked me,

Not really clear on what you mean by the $buf of $it = $nbuf; part, is this for inheritance? If so, just expose $buf as a method and have subclasses use that?

I wanted a class method to set the $buf slot of some object. Yes, I might need to expose $buf as a method.

You also showed how I can access slots from a BUILD block. Corinna wouldn't have a BUILD block, so I would use an ADJUST block. Object::Pad also has ADJUST, so I can try it now. Here, I have a BUILD block to read the first 10 bytes of a gzip file (RFC 1952),

use Object::Pad;

class GzipHeader;
use Carp qw(croak);
use constant SIZE => 10;

has $buf;

BUILD ($fh) {
  read($fh, $buf, SIZE) == SIZE or croak "short header";
}

method cm() { unpack('x2 C', $buf) }
method os() { unpack('x9 C', $buf) }

package main;                                                                   
use IO::File;
for (@ARGV) {
  my $fh = IO::File->new($_, '<');
  my $header = GzipHeader->new($fh);
  printf "$_: compression method %s, operating system %s\n",
    $header->cm, $header->os;
}

I change it to an ADJUST block,

has $fh: param;
has $buf;

ADJUST {
  read($fh, $buf, SIZE) == SIZE or croak "short header";
  undef $fh;
}
...
  my $header = GzipHeader->new(fh => $fh);

This is awkward. GzipHeader->new can only put values in slots, so I need an extra slot $fh just for the ADJUST block. Maybe I should not use ADJUST block in this way.

Ovid commented 3 years ago

This is resolved for the MVP. Further issues should be new tickets. People should consult the README for specifics on the RFC and address the different portions. The documents are not considered final.