Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rationale Feedback #3

Closed
Ovid opened this issue Mar 22, 2020 · 16 comments
Closed

Rationale Feedback #3

Ovid opened this issue Mar 22, 2020 · 16 comments
Labels
Feedback Cor Proposal Feedback

Comments

@Ovid
Copy link
Collaborator

Ovid commented Mar 22, 2020

Use this issue to leave feedback on the Cor Rationale.

@Ovid Ovid added the Feedback Cor Proposal Feedback label Mar 22, 2020
@openstrike
Copy link

Is it too late to change the name?

The fact that it sounds like "core" is a happy coincidence.

We already have some (thankfully limited) confusion between core and CORE. To now add Cor to the mix isn't going to help. Try explaining to someone new to Perl that Cor is in core but not in CORE and they are most likely to run screaming back to python.

@Ovid
Copy link
Collaborator Author

Ovid commented May 20, 2020

@openstrike This has complicated some video calls I've had with Sawyer and Stevan, so I know where you're coming from. When that happens, we just refer to it by its full name: Corinna.

@openstrike
Copy link

Indeed, "Corinna" would be much less ambiguous. Thanks.

@haarg
Copy link

haarg commented May 22, 2020

The Goals section doesn't match what is stated on the home page.

@matthewpersico
Copy link

matthewpersico commented Jun 1, 2020

My apologies for not chiming in sooner, but I've been surprisingly busier than when I had to commute for a living. This comment will deal with this code:

    has $cache :handles(get) :builder;
    method _build_cache () { Hash::Ordered->new }
  • I'd replace handles with provides. I have no immediate understanding of the details of what "handling" is but I know exactly what "providing" is: $cache provides its own get so don't synthesize one, just call get on the object in the slot. And if the method is not called get, provide (no pun intended) its name like :provides(get:read).

  • I'd replace _build_cache with _builder_cache. The mapping between ':builder" and '_build' doesn't quite hit me as obviously on first read as ':builder" and '_builder' would. And even that doesn't feel quite right. I wouldn't mind typing :builder(q(_build_cache)) as a positive link between the function and its purpose. I hate, hate, hate when frameworks glue stuff together behind the scenes and depend on naming conventions (see recent Python rants). I can live with :reader meaning get() and :writer meaning set() but I prefer more complicated stuff to be stated.

  • The first class you mention violates one of your own complaints about documentation - easy read examples. If you are would like to introduce us to Corinna with an example containing the attributes :new, :reader, and :writer, put in a paragraph explaining quickly that for attribute foo, :new means you can provide a value on construction, :reader means there will be a get_foo() provided for you and :writer means there will be a set_foo() provided for you. And if I am wrong (foo() is the get() and foo(val) is the set()?) then it's because I haven't read that far yet and I am guessing, which, I think, bolsters my point.

More to follow as I read.

@matthewpersico
Copy link

  has $cache :handles(get) = Hash::Ordered->new;

To me, this says "the cache slot handles gets by calling Hash::Ordered->new". Obviously that's wrong. It's supposed to mean that the cache slot is initialized with a call to Hash::Ordered->new. This looks too much like Python's bolted on mypy nonsense sprinkling type info in between declaration and assignment. Please keep the things that are related visually textually together:

  has $cache = Hash::Ordered->new :handles(get);

If that's a parsing nightmare, then maybe

  has $cache :initer(Hash::Ordered->new) :handles(get);

@matthewpersico
Copy link

has $created :reader = time;

In the above, we have a new :reader attribute which says "create a read-only method name created to read this value. But if you want a different method name, that's fine: :reader(get_created).

See? In #3 (comment), I guessed readers were defined as *get*_slot() and not slot() I was wrong...

@matthewpersico
Copy link

You don't see it in the above code, but you automatically get a $self variable in a method if you declare it with method.

Every OO language I know has some kind of explicit 'self' expression. I wonder how many people are going to blanche at not being able to tell their slots from other variables at first glance without a $self and do (something_like) this:

class Customer {
    has $s_name      :isa(Str)      :new :reader(name) :writer(name);
    has $s_birthdate :isa(DateTime) :new :reader(birthdate) :writer(birthdate);
}

where s_ is the stand-in for self.

@duncand
Copy link

duncand commented Feb 26, 2021

Why call it version v0.01.0 rather than v0.1.0? Multi-dot notation is explicitly a series of integers for whom leading zeroes don't change the meaning. If you actually want to overlay with Perl's native floating point number versions, then it would be 0.001000.

@Ovid
Copy link
Collaborator Author

Ovid commented Feb 26, 2021

@duncand Version switched to v0.1.0. That's just me being silly. Thanks for catching that.

@duffee
Copy link

duffee commented Mar 4, 2021

The assertion that Cor is better than Moo/se is missing supporting arguments beyond performance/core-ness (i.e. non-syntax issues). IMHO, the short class definition (I mean the Constructor attributes ) is not going to win any converts to Perl because it is too dense. If we as readers didn't need white space, there would be no /x regex modifier. The syntax for Corinna is great for writing, horrible for reading because it ends up being a wall of text. So the question is, who is your audience? Experienced perlers or new converts?

My suggestion for post-lockdown is to temp some Javascript developers with coffee and pastry to do some A/B/C testing. They won't freak out over $variables, they haven't used Moo/se before and people will agree to anything when pumped up on caffeine and sugar. Show them a simple class with a view attributes, a couple of clear methods and a frobobnicate method written up in bless, Moose and Corinna style as you've done in the Rationale. Ask them what the frobobnicate method does just to get them to engage with the class and then get them to rank the styles in order of clarity. Print the classes out on separate pieces of paper so they can be viewed side by side and shuffled around or scribbled on.

Both bless and frobobnicate are distractors. You're trying to warm up your subjects so they relax and can give you their gut instinct on the syntax. You don't want them to overthink the differences between Moose and Corinna or you'll get bad data. My guess is that Moose will rank highest for clarity, but what if the sweet spot is somewhere between? Well then, time for A/B/C/D/E testing which is starting to resemble the Card sorting method of elicitation. Take a single feature from one syntax and drop it in the other so that you can identify the best combination of features.

All of this side steps the issue of how easy it is to maintain, but with data, you stand a better chance of convincing p5p.

@duncand
Copy link

duncand commented Mar 4, 2021

One thing that's super-important when asking opinions on syntax is to use examples that are as realistic as possible, the kinds of classes that would be commonly written in real work intended for a real production system.

So none of that "Point" class stuff having artificially short member names and no practical use.

It would need to have member names with realistically descriptive lengths and quantities and variety.

@Ovid
Copy link
Collaborator Author

Ovid commented Mar 5, 2021

@duffee wrote:

IMHO, the short class definition (I mean the Constructor attributes ) is not going to win any converts to Perl because it is too dense. If we as readers didn't need white space, there would be no /x regex modifier. The syntax for Corinna is great for writing, horrible for reading because it ends up being a wall of text.

Then add whitespace.

class Cache::LRU {
    use Hash::Ordered;

    has $cache
        :handles(get)
        = Hash::Ordered->new;

    has $max_size
        :new
        :reader
        = 20;

    has $created
        :reader
        = time;

    method set ( $key, $value ) {
        if ( $cache->exists($key) ) {
            $cache->delete($key);
        }
        elsif ( $cache->keys > $max_size ) {
            $cache->shift;
        }
        $cache->set( $key, $value );  # new values in front
    }
}

Seriously, I'm unsure if I'm missing the point or not. If is => 'rwp' is better than :reader because it has more whitespace, can you explain why? Why is handles => [ qw/ get set / ] better than :handles( get, set )?

Or an example similar to what I use in talks:

has volume => (
    is       => 'ro',
    init_arg => undef,
    builder  => '_build_volume',
);

sub _build_volume($self) {
    return $self->height * $self->width * $self->depth;
}

Why is the above preferable to has $volume :reader = $height * $width * $depth; other than as a "style" thing?

Or you can do:

has $volume
    :reader
    = $height * $width * $depth;

I'm not trying to be difficult. I genuinely don't understand the issue here (also, post-lockdown is likely a year or more away).

@duncand
Copy link

duncand commented Mar 5, 2021

Right on Ovid, I find your Corinna version nicer than the Moo* ones.

@abraxxa
Copy link

abraxxa commented Mar 5, 2021

I prefer Moo(se) because the key/value pairs are better readable.

@Ovid
Copy link
Collaborator Author

Ovid commented Aug 15, 2021

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

@Ovid Ovid closed this as completed Aug 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feedback Cor Proposal Feedback
Projects
None yet
Development

No branches or pull requests

7 participants