Tau.feat.pr03.ghi 005

[tabulon]

Hi Perlancar,

Thanks a lot for making the time for these PR's, which probably have distracted whatever you had expected to have been doing.

This PR hopefully resolves #5, adopting the behaviour we have disscussed and agreed upon on fringe comments for an other issue, (https://github.com/perlancar/perl-Getopt-Long-More/issues/1#issuecomment-455994018) earlier today.

I am finally going ahead and submitting those changes in an "optimistic" expectation, based upon the evolution of that thread, even though I haven't heard your final feedback on the commit that I had made available for your review in that midst.

As you will notice, this submitted PR adds a 3rd commit, which carries very slight changes to the version that was out for review, namely:

• 1) More.pm now prefers to die rather than warn when faced with a situation where it cannot possibly honor a validation constraint that it encounters for an OptSpec that lacks a handler, which may be sumarized to the following condition (although the actual code is scattered):

  die if !exists $->{handler} && ( $->{required} || exists $_->{default} )`

  (BTW, I should have added a couple tests in there that ensure the above described behaviour, but somehow I could not manage to make that happen) Perhaps you could code those, or briefly explain how I could go about it, either now or later?

• 2) I have now removed the -uncessary- attempt for a test that tried (unsuccesfully) to ensure GoL was dies with Getopts (<> => optspec() ).

Also, please acknowledge that you you have taken note of the new bug that is, for now, hastily signaled in the fringe comments of the test suite. I am preparing an proper issue for it that will be entitled something like Avoid inadvertently hijacking GoL's legacy "default destinations".

That's it for now.

Let me know if anything in the submitted PR raises an eyebrow or comes as a surprise (I don't see any reason, based on my understanding of our discussion, but you never know).

Cheers!

---

N.B. :

One last thing: In the general case, I do fully agree with your remarks about early failure, and to a lesser extent, the docs.

However, in this particular case, I also have something else in mind ... in addition to to your parenthetical observation, which is probablly already sufficient to label this as something "that could be stretched to go either way", dpending on what else is at stake, .. (more on this later, if you wish).

[perlancar]

Also, please acknowledge that you you have taken note of the new bug that is, for now, hastily signaled in the fringe comments of the test suite. I am preparing an proper issue for it that will be entitled something like Avoid inadvertently hijacking GoL's legacy "default destinations" .

Noted. So the fix would be to modify $Getopt::Long::caller so GLM's caller will be the target instead of GLM, correct? I'll let you handle that.

[perlancar]

More.pm now prefers to die rather than warn when faced with a situation where it cannot possibly honor a validation constraint that it encounters for an OptSpec that lacks a handler, which may be sumarized to the following condition (although the actual code is scattered):

die if !exists $->{handler} && ( $->{required} || exists $_->{default} )`

Let me see if I understand what you're getting at here. You want to make sure that if an option has a default property, the default can be fed to the handler, right? But note that the handler is unnecessary if we are in "hash storage mode". Also, why the need for the handler to exists when 'required' is true? When user does not supply an option that is 'required', shouldn't we just die?

[perlancar]

When user does not supply an option that is 'required', shouldn't we just die?

Ah never mind. We pass to GoL first before checking for required option, so we need to check that the handler is indeed accessed. Then that only leaves the issue of dying when there is no handler, option is required, but we are in hash storage mode.

[tabulon]

Yes, that's exactly it.

I'll take a stab at it.

On 21 Jan 2019, at 18:09, perlancar wrote:

Also, please acknowledge that you you have taken note of the new bug that is, for now, hastily signaled in the fringe comments of the test suite. I am preparing an proper issue for it that will be entitled something like Avoid inadvertently hijacking GoL's legacy "default destinations" .

Noted. So the fix would be to modify $Getopt::Long::caller so GLM's caller will be the target instead of GLM, correct? I'll let you handle that.

-- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://github.com/perlancar/perl-Getopt-Long-More/pull/7#issuecomment-456143437

[perlancar]

(BTW, I should have added a couple tests in there that ensure the above described behaviour, but somehow I could not manage to make that happen) Perhaps you could code those, or briefly explain how I could go about it, either now or later?

Tests added in 4c87a11.

[tabulon]

Great!

"TODO" tests also helped me better understand your previous comments.

-- which had switched me into a epic-authoring mode (read: crunching miles of boring text :-)

The main point of that long story can actually be expressed by a modified name from one of your test cases :

- ... we shouldn't require handler _(even in 'classic' mode)_ -> 

DWIM.

I will send out that anyway, as it has other points in it, for you to skim-through later; and also for future stumblers.

Meanwhile the following two additional test cases should suffice at least to communicate the gist (even if they are not added to the test suite):

TODO: {
     local $TODO = "currently dies, but we shouldn't require handler 
(even in 'classic' mode)";
     my $opts = {};
     test_getoptions(
         name => 'optspec: default (set, but no handler) -> DWIM',
         opts_spec => ['foo=s' => optspec(default => "bar")],
         argv => [qw/--foo qux/],
         opts => $opts,
         expected_opts => {foo => "qux"},
         expected_argv => [qw//],
     );
}

TODO: {
     local $TODO = "currently dies, but we shouldn't require handler 
(even in 'classic' mode)";
     my $opts = {};
     test_getoptions(
         name => 'optspec: required (set, but no handler) -> DWIM',
         opts_spec => ['foo=s' => optspec(required => 1)],
         argv => [qw/--foo qux/],
         opts => $opts,
         expected_opts => {foo => "qux"},
         expected_argv => [qw//],
     );
}

Obviously, these are 100% in contradiction with the two other "dies" tests you have just added; which just reflect the current "more modest" expectations; Once we get around to coding this, the "dies" ones should simply be deleted.

I am stating to think that the whole misunderstanding was due to my lack of precision, in the below quoted request. Sorry about that...

---

Cheers

On 22 Jan 2019, at 7:26, perlancar wrote:

(BTW, I should have added a couple tests in there that ensure the above described behaviour, but somehow I could not manage to make that happen) Perhaps you could code those, or briefly explain how I could go about it, either now or later?

Tests added in 4c87a11.

-- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://github.com/perlancar/perl-Getopt-Long-More/pull/7#issuecomment-456285178

[tabulon]

Re-posting my previous comment. It was messed up becasue it had been posted via email.

Great!

Those "TODO" tests helped me better understand your previous comments.

Those comments had switched me into a epic-authoring mode (read: crunching miles of boring text :-)

The main point of that long story can actually be expressed by a modified name from one of your test cases :

• ... we shouldn't require handler (even in 'classic' mode) -> DWIM.

I will send out that anyway, as it has other points in it, for you to skim-through later; and also for future stumblers.

Meanwhile the following two additional test cases should suffice at least to communicate the gist (even if they are not added to the test suite):

TODO: {
     local $TODO = "currently dies, but we shouldn't require handler 
(even in 'classic' mode)";
     my $opts = {};
     test_getoptions(
         name => 'optspec: default (set, but no handler) -> DWIM',
         opts_spec => ['foo=s' => optspec(default => "bar")],
         argv => [qw/--foo qux/],
         opts => $opts,
         expected_opts => {foo => "qux"},
         expected_argv => [qw//],
     );
}

TODO: {
     local $TODO = "currently dies, but we shouldn't require handler 
(even in 'classic' mode)";
     my $opts = {};
     test_getoptions(
         name => 'optspec: required (set, but no handler) -> DWIM',
         opts_spec => ['foo=s' => optspec(required => 1)],
         argv => [qw/--foo qux/],
         opts => $opts,
         expected_opts => {foo => "qux"},
         expected_argv => [qw//],
     );
}

Obviously, these are 100% in contradiction with the two other "dies" tests you have just added; which just reflect the current "more modest" expectations; Once we get around to coding this, the "dies" ones should simply be deleted.

I am stating to think that the whole misunderstanding was due to my lack of precision, in the below quoted request. Sorry about that...

---

Cheers

[tabulon]

Below is that epic story referred to in here and there

I mean epic literally; who wants to read 10 miles of text? -- for something that can be said in a few words.

---

Almost.

I believe our understanding differs only slightly, namely regarding the following point :

• What does hash-storage mode have to do anything with this whole thing? -- that is, except somewhat facilitating the task of figuring out the actual destination/linkage that has been designated by GoL; and only if/when GLM has an actual need to do so.

SUMMARY

As a reminder, as far as GoL is concerned, at the end of the day, an option will always end up getting a designated destination (i.e. linkage); whatever the mode (classic or hash-storage), and whatever the means for that designation ("implicit" or "explicit").

In fact, afaics, when GoL::Getoptions can't figure one out for an option (explicitly or implicitly), as in the case of a messed up arguments to Getopts*(), it will simply die.

It's just that:

• a) sometimes GLM already has direct knowledge (of the designated destination), i.e. when it explicitly provided it to GoL in the first place, e.g. via push @go_params $ospec->{handler}

• b) And sometimes, GLM simply doesn't know; and then it's a matter of 1) how easy/hard to figure it out; and 2) how confident it can be of what it can figure out.

  Imho, the question of mode only comes into play here, where hash-storage mode shines (for both ease and confidence); whereas classic remains quite doable, too.

  Both require parsing the option spec ($osname), though; and that's where I would be more reluctant.

And also (orthogonally):

• Sometimes GLM doesn't even need that knowledge (i.e. unless ( $ospec->{required} || exists $ospec->{default} )
• Sometimes GLM can't do much with that knowledge after all even when it knows (or figures out) what it is; e.g. if ( ref ($dest) =~ /CODE )

---

GORY DETAILS

Below is detailed break up of the cases that are worth distinguishing, imho.

case 0) When knowledge is UNNECESSARY

This refers to the case where GLM has no need to know the destination, and corresponds to the following conditions, given GLM's current feature-set:

a) : unless ( $ospec->{required} || exists $ospec->{default} )
b) : exists $ospec->{default} && !defined $ospec->{default}

The latter (b) is an edge case (which reflects current GLM code) that would need further discussion. What do you think?

Note: In reality, some of the expressions above should probably contain extra exists guards to protect against autovivification, ....

case 1) When knowledge is READILY AVAILABLE

This is a trivial case where everything is easy.

Here, GLM believes to have DIRECT KNOWLEDGE of the designated destination.

It currently applies when GLM has provided the destination explicitly to GoL in the first place, via the gross equivalent of :

push @go_params $handler if exists $ospec->{handler}

where $handler is either $ospec->{handler} or its wrapped cousin (for <>)

case 2) When knowledge is OBTAINABLE

In all other cases, GLM doesn't really know the destination in a direct manner, but could potentially try to figure it out, in varying degrees of difficulty and hairiness:

• 2.a) Easy : HASH-STORAGE mode

  In hash-storage mode, GLM can figure out the implicit destination relatively easily.

  That is, as long as we are willing to parse the spec ($osname) to get at the real name of the option ($orig in GoL code), thereby slightly duplicating work (if not code) of GoL.

  This is the remaining issue you are mentioning, I believe

• 2.b) Hairier: CLASSICAL mode

  In classical mode, things are a slightly more complicated, because the designated (implicit) destination, in this case, would be a global variable in caller's package namespace.

  Incidentally, this has to do with the "legacy default destination" scenario where GLM currently has a bug, resulting in those variables to come into existence in GLM package namespace, which would preclude us from doing anything about this case.

  Anyhow, as this a "deprecated" scenario for GoL, so it should probably get less priority.

SIDE NOTE

Whether default destination should have been deprecated by GoL in the first place is another story, though...

Even today, I believe it's one of those things that could make all the difference in someone's mind when it's time to decide (in 5 seconds) if they are going to type #!/bin/bash or #!/usr/bin/env perl for their next "quick and dirty" "throw away" "shell script"

(especially so for people experienced enough to suspect any or all of those expressions in quotes :-).

... It's one thing having to "refactor" the perl code later, if ever needed. ... And a complete other story if you first have to translate bash => perl, which won't happen very often. ...So you end up having to put up with an ever growing bash script... and each added line over time pulls you in even more into the sad trap....

case 3) When knowledge ends up being USELESS

This is when GLM can't do much with a destination, even when it already knows (or figured out) what it is:

e.g. : ref ($dest) =~ /CODE

Need to discuss what should happen when GLM also happens to "need" (i.e. have been instructed) to do something with that dest:

i.e. : ... and ( $ospec->{required} || exists $ospec->{default} )

What do you think GLM should do in this situation : die?, warn?, or ignore.

Note: Again, some of these expressions should probably contain exists guards to protect against autovivification, ....

[tabulon]

Here's another passage from the long story that might be of interest:

Imho, the question of mode only comes into play here, where hash-storage mode shines (for both ease and confidence); whereas classic remains quite doable, too.

Both require parsing the option spec ($osname), though; and that's where I would be more reluctant.

[perlancar]

I'll ask more questions first to ensure that I understand things correctly.

case 0) When knowledge is UNNECESSARY a) : unless ( $ospec->{required} || exists $ospec->{default} )

Why is this condition categorized into case 0)? When there is a default value, GLM is the one who needs to assign the default value to the linkage, thus GLM needs to know the linkage.

[tabulon]

Note the preciding unless...

So, the whole expression is actually: ! ( $ospec->{required} || exists $ospec->{default} )

[perlancar]

Ah okay. Missed that. Next item:

case 3) When knowledge ends up being USELESS This is when GLM can't do much with a destination, even when it already knows (or figured out) what it is: e.g. : ref ($dest) =~ /CODE Need to discuss what should happen when GLM also happens to "need" (i.e. have been instructed) to do something with that dest:

i.e. : ... and ( $ospec->{required} || exists $ospec->{default} )

What do you think GLM should do in this situation : die?, warn?, or ignore.

I'm thinking GLM could invoke the coderef, like GoL does. That can be considered as just another way to do the "linking". Although since the coderef might expect the Getopt::Long::CallBack object as the first destination, we will need to construct one two, which might or might not be cumbersome (haven't looked at the GoL code). What do /you/ think?

[tabulon]

Yes, you are right; it's indeed possible wrap CODE before calling inner (similar to the <> case now); in which case it would be worth generalizing that a bit.

The GOL contact would be relatively easy to honor.

that code has already been called by GoL

On 22 Jan 2019, at 14:16, perlancar wrote:

Ah okay. Missed that. Next question:

case 3) When knowledge ends up being USELESS This is when GLM can't do much with a destination, even when it already knows (or figured out) what it is: e.g. : ref ($dest) =~ /CODE Need to discuss what should happen when GLM also happens to "need" (i.e. have been instructed) to do something with that dest:

i.e. : ... and ( $ospec->{required} || exists $ospec->{default} )

What do you think GLM should do in this situation : die?, warn?, or ignore.

I'm thinking GLM could invoke the coderef, like GoL does. That can be considered as just another way to do the "linking". Although since the coderef might expect the Getopt::Long::CallBack object as the first destination, we will need to construct one two, which might or might not be cumbersome (haven't looked at the GoL code). What do /you/ think?

-- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://github.com/perlancar/perl-Getopt-Long-More/pull/7#issuecomment-456395325

[tabulon]

Please Ignore the last sentence that reads: "that code has already been called by GoL" (it stems from a prior reflex because I had misunderstood your comment, I think)

On 22 Jan 2019, at 15:25, Ayhan Ulusoy wrote:

Yes, you are right; it's indeed possible wrap CODE before calling inner (similar to the <> case now); in which case it would be worth generalizing that a bit.

The GOL contact would be relatively easy to honor.

that code has already been called by GoL

On 22 Jan 2019, at 14:16, perlancar wrote:

Ah okay. Missed that. Next question:

case 3) When knowledge ends up being USELESS This is when GLM can't do much with a destination, even when it already knows (or figured out) what it is: e.g. : ref ($dest) =~ /CODE Need to discuss what should happen when GLM also happens to "need" (i.e. have been instructed) to do something with that dest:

i.e. : ... and ( $ospec->{required} || exists $ospec->{default} )

What do you think GLM should do in this situation : die?, warn?, or ignore.

I'm thinking GLM could invoke the coderef, like GoL does. That can be considered as just another way to do the "linking". Although since the coderef might expect the Getopt::Long::CallBack object as the first destination, we will need to construct one two, which might or might not be cumbersome (haven't looked at the GoL code). What do /you/ think?

-- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://github.com/perlancar/perl-Getopt-Long-More/pull/7#issuecomment-456395325

[tabulon]

I am suspecting this is heading towards something along the lines of the sketch below, if we are willing to pay a small price in performance.

What do you think ?

package Getopt::Long::More;

sub GetOptionsFromArray {

  { ... }

  MAPPING:  # Munging / Evaporation
      for my $e (@opts_spec) {

        if ( ref($e) && eval { $e->swearing_to_be_a_GLM_compatible_OptSpec_thingy() } ) {
          $e->spec($prev);
          push @go_opts_spec, sub { $e->sandwich_handler(@_) } if $e->has_handler();

        } else {
          push @go_opts_spec, $e;
        }

    } continue {
      $prev = $e;
    }

  COMPLETION: {
   ...
  }

  INNER_CALL:
    my $res = Getopt::Long::GetOptionsFromArray($ary, @go_opts_spec);

  VALIDATION: {
    for (@opts_spec) {
      if ( ref($_) && eval { $_->swearing_to_be_a_GLM_compatible_OptSpec_thingy() } ) ) {
        $_->validate(...)
      }
  }

  $res
}

...

package # hide from PAUSE indexer
    Getopt::Long::More::OptSpec;

use Object::Tiny qw(...); # Get some very fast accessors for very cheap.
                          # Alternatively, copy over Tiny's 30 lines of code :-)

# NOTE : Names are obviously just made up for now :-)

sub has_handler { exists %{ shift }{handler} }    #
sub swearing_to_be_a_GLM_compatible_OptSpec_thingy { 1 }

sub sandwich_handler {
  my $self = shift;
  my $handler;

  die "WTF" unless $handler = $self->{handler}; # Undef is a no-go, here

  # sneak preview & eventually save the value, whatever
  $self>encountered( $self>encountered() + 1 );

  ...

  my $res= $handler->(@_);

  # maybe something else, later on
  ...

  $res
}

sub validate {
    my $self = shift;
    my $spec = $self->spec;

    # equivalent of what currently happens inside the validation loop
    ...

}

[tabulon]

You will notice that the following check is missing from the above at this time:

 ( reftype($handler) eq 'CODE' )

It's because there are several places where it could go; depending...

Naturally, it always needs checked within sandwich_handler()

But MAPPING may or may not need it; depending on just how much sandwich_handler() is expected to accomplish....

The simplest way to go and the one I would vouch for (at least for now) would be do the same check in MAPPING, as well, like in:

   ...
   $e->spec($prev);
   my $handler =   $e->has_handler() && $e->handler();
         $handler =   sub { $e->sandwich_handler(@_) } if reftype ($handler)  eq 'CODE' ;
   push @go_opt_spec, $handler
   ....

[tabulon]

As you'll notice, that swearing_to_be_a_GLM_compatible_OptSpec_thingy method accomplishes something quite similar to cheking for a Role, but without the ton of bloat that comes with pretty much any implementation around.

It also conveys a wish for OptSpec to be inheritable and/or replacable....

[tabulon]

Also, there probably is a slight issue with the sketch above: namely concerning the 'encountered' attribute.

So far, the purpose of optSpec is purely "descriptive" :-) As such, it contains only metadata about an option; no "run-time" state.

And it should probably stay that way... It's a sane approach that's already there. No reason to stain it.

But it should be relatively easy to shove that info somewhere else than the OptSpec object itself.

---

The same is not true for spec, though. It can really belong there.

.

[tabulon]

Here's the same mock/sketch, minus some stupid mistakes + the CODE checks happening as suggested above. Did not touch the "encountered" part, yet.

package Getopt::Long::More;

sub GetOptionsFromArray {

  {...}

  MAPPING:
      for my $e (@opts_spec) {

        if ( ref($e) && eval { $e->swearing_to_be_a_GLM_compatible_OptSpec_thingy() } ) {

          $e->spec($prev);

          if ( $e->has_handler() ) {
            my $handler = $e->handler();
            $handler    = sub {; $e->sandwich_handler(@_) } if reftype ($handler)  eq 'CODE' ;
          }

          push @go_opt_spec, $handler;
        } else {
          push @go_opts_spec, $e;
        }

    } continue {
      $prev = $e;
    }

  COMPLETION: {
   ...
  }

  INNER_CALL:
    my $res = Getopt::Long::GetOptionsFromArray($ary, @go_opts_spec);

  VALIDATION: {
    for (@opts_spec) {
      if ( ref($_) && eval { $_->swearing_to_be_a_GLM_compatible_OptSpec_thingy() } ) {

        $_->validate();
      }
    }
  }

  $res
}

{...}

package # hide from PAUSE indexer
    Getopt::Long::More::OptSpec;

use Object::Tiny qw(handler required default spec summary description completion x _ );
                          # Get some very fast accessors for very cheap.
                          # Alternatively, copy over Tiny's 30 lines of code :-)

# NOTE : Names are obviously just made up for now :-)

sub has_handler { my $self = shift; exists $self->{handler} }
sub swearing_to_be_a_GLM_compatible_OptSpec_thingy { 1 }

sub sandwich_handler {
  my $self = shift;
  my $handler = $self->handler;

  die "WTF" unless $handler && ( reftype ($handler) eq 'CODE' );

  # Mark the encounter
  $self->encountered( $self->encountered() + 1 );

  {...} # maybe some other stuff (sneak preview?)

  my $res = $handler->(@_);

  {...} # maybe something else, later on

  $res
}

sub validate {
    my $self = shift;
    my $spec = $self->spec;

    # equivalent of what currently happens inside the validation loop
    {...}

}

[tabulon]

Never mind gibberish above on encountered: In most cases handler is already runtime data....

So it's already somehow stained with runtime data;

The evil introduced by the sketch above is mutation, rather....

[tabulon]

Still doesn't feel right, though.