As promised, here is an initial PR, albeit this one is not for the original issue reported (#1), but another one (#3) that blocks the resolution of the former.
The PR also adds a bunch of tests for #1, where those currently failing are properly maeked as TODO.
Below, I am ust copying the message from the latest commit; no need to add more length to a message that's already unnecssarily long and detailed... Sorry about that :-)
The WHY:
Although GLM () aims and claims to be backward
compatible with the long-time veteran GoL module, in
practise there remained some incompatibilities for some "legal"
(and even somewhat popular) GoL usage patterns.
Two of such cases are described Githubs issues #1 and #3 as
outlined below. This commit hopefully fixes #3, which also blocked
resolving #1, for which a simple fix is on its way.
Imho, it is quite important to nail down and fix such cases if we
want GLM to be a viable smooth migration path of legacy code,as well
as enabling more DRY & DWIM.
The WHAT:
This commit hopefully resolves [#3][implicit handlers/linkages],
and also paves the way for resolving [#1][proper hash-storage support]
In fact, the latter was discovered while working on the former.
PREVIOUS BEHAVIOUR
As a reminder, the "old" behaviour (which is at present still the
"current" behaviour on repo Master unless this commit is merged),
relies upon the -false- assumption that option definitions would
always come pairs of $spec => $linkage (aka $handler).
SIDE NOTE ON TERMINOLOGY AND BACKGROUND
As a side note, the GLM term "handler" appears to correspond pretty
much exactly to the term "linkage" used by GoL ().
As opposed to the "spec", which describes what is "legal" for an
option (name, aliases, type, etc, ...), the "handler" (or "linkage")
somehow determines what will be done with an option/flag once it is
found in @argv (or equivalent).
Unsurprisingly, a variety of 'ref' types (SCALAR, CODE, HASH,...)
are allowed and supported by GoL as possible "linkage" thingies, and
GLM passes the ball onwards to GoL in this respect, without
nit-picking and over-validating what is allowed and what is not.
(which is a good thing, imho).
There are two scenarios where behaviour in this matter (i.e.
what is acceptable as a valid "linkage"/"handler") diverge
significantly between GoL and GLM :
1) Slight divergence in design (NOT TOUCHED BY THIS COMMIT)
GoL happily accepts the situation where the "linkage" is not
provided for a given "spec" (and takes it as an opportunity)
to offer more DRY & DWIM);
Whereas GLM goes ahead and forbids this case by refusing to
create an OptSpec object without a defined "handler"; thereby
preventing any such DRY & DWIM that could have been natively
provided even by GoL itself.
2) GLM bug #3 (HOPEFULLY FIXED BY THIS COMMIT)
The GLM wrapper for "GetOptionsFromArray()" gets "confused"
when presented with a list of option definitions that don't
come in pairs of (spec => "linkage"), even outside of any
OptSpec(), as soon as there has been at least one OptSpec
"seen".
While point (1) is clearly a design choice that would need further
discussion before any action, point (2) above was clearly a bug,
described in more functional detail under github issue #3, and now
hopefully fixed by this commit.
The bug was somewhat sneaky though, in that it would trigger only
under certain circumstances, especially depending whether or not
there was at least one OptSpec object passed in as a "linkage"
in the actual arguments; and also some "chance" factor seemingly
coming into play that had to with "% 2" math... :-)
But once "confused", GLM's wrapper for "GetOptionsFromArray()"
would simply pass a wrong list of arguments to its wrapped
GoL counterpart, causing the latter to "die" in a manner quite
unexpected by module user, throwing a cryptic message difficult
to comprehend.
Please note that, so far, this still does not resolve
github issue [#1][proper hash-storage support]. On the other
hand, it paves the way for its resolution and actually renders
that fix to become quite trivial.
It would also allow and simplify revisiting the design choice
mentioned in point (1) above; but that would be for an other
PR, depending on how the discussion evolves.
THE NEW BEHAVIOUR
The new behavior is actually a simplification, both functionally
and code-wise.
Basically, we do away with the reliance on the above mentioned
assumption of ( spec => "linkage") argument pairs.
We don't even try to do any "smart" distinction between "specs"
and "linkages" received as arguments, because that would either
require reproducing a hunk of GoL code in the wrapper, or else
prohibit stringified
Instead, we just limit ourselves to dealing with any encountered
OptSpec objects themselves, passing anything else untouched
to our wrapped GoL counterpart, who becomes re-enabled to do its
own DRY & DWIM.
The HOW:
Added a few test cases to cover this fix (for gh #3) as well as
the next one coming up that handles gh #1.
Some were already passing, some others had to be marked as "TODO"
to prevent any adverse impact on current builds on Master.
The actual changes to the code were limited to two loops within
the wrapper routine for "GetOptionsFromArray". Both loops had to
be modified to get rid of the "% 2" assumption described above.
Here are broad locations of these loops:
1) The initial loop entitled "# strip the optspec objects"
2) The post-processing loop (just after the call to the wrapped
counterpart)
Unfortunately, a surgical modification of the first loop was not
quite feasible, so I had to resort to some code reorganisation,
paying close attention to preserving all the intended behaviour.
As a slight bonus, we now have less code in there.
The impact on performance was not measured, but I would not
expect any adverse impact in that department; if anything, a very
slight (minute, rather) gain can be expected.
Modifying the latter loop (2) was quite simple, just got rid of
the ($i % 2) expression, replacing it with a less error prone
$i > 0 enough to protect against an incorrect array access.
Hi Perlancar,
As promised, here is an initial PR, albeit this one is not for the original issue reported (#1), but another one (#3) that blocks the resolution of the former.
The PR also adds a bunch of tests for #1, where those currently failing are properly maeked as TODO.
Below, I am ust copying the message from the latest commit; no need to add more length to a message that's already unnecssarily long and detailed... Sorry about that :-)
The WHY:
Although GLM () aims and claims to be backward
compatible with the long-time veteran GoL module, in
practise there remained some incompatibilities for some "legal"
(and even somewhat popular) GoL usage patterns.
Two of such cases are described Githubs issues #1 and #3 as outlined below. This commit hopefully fixes #3, which also blocked resolving #1, for which a simple fix is on its way.
Imho, it is quite important to nail down and fix such cases if we want GLM to be a viable smooth migration path of legacy code,as well as enabling more DRY & DWIM.
The WHAT:
This commit hopefully resolves [#3][implicit handlers/linkages], and also paves the way for resolving [#1][proper hash-storage support] In fact, the latter was discovered while working on the former.
PREVIOUS BEHAVIOUR
As a reminder, the "old" behaviour (which is at present still the "current" behaviour on repo Master unless this commit is merged), relies upon the -false- assumption that option definitions would always come pairs of $spec => $linkage (aka $handler).
SIDE NOTE ON TERMINOLOGY AND BACKGROUND
As a side note, the GLM term "handler" appears to correspond pretty much exactly to the term "linkage" used by GoL ().
As opposed to the "spec", which describes what is "legal" for an option (name, aliases, type, etc, ...), the "handler" (or "linkage") somehow determines what will be done with an option/flag once it is found in @argv (or equivalent).
Unsurprisingly, a variety of 'ref' types (SCALAR, CODE, HASH,...) are allowed and supported by GoL as possible "linkage" thingies, and GLM passes the ball onwards to GoL in this respect, without nit-picking and over-validating what is allowed and what is not. (which is a good thing, imho).
There are two scenarios where behaviour in this matter (i.e. what is acceptable as a valid "linkage"/"handler") diverge significantly between GoL and GLM :
1) Slight divergence in design (NOT TOUCHED BY THIS COMMIT)
GoL happily accepts the situation where the "linkage" is not provided for a given "spec" (and takes it as an opportunity) to offer more DRY & DWIM);
Whereas GLM goes ahead and forbids this case by refusing to create an OptSpec object without a defined "handler"; thereby preventing any such DRY & DWIM that could have been natively provided even by GoL itself.
2) GLM bug #3 (HOPEFULLY FIXED BY THIS COMMIT)
While point (1) is clearly a design choice that would need further discussion before any action, point (2) above was clearly a bug, described in more functional detail under github issue #3, and now hopefully fixed by this commit.
The bug was somewhat sneaky though, in that it would trigger only under certain circumstances, especially depending whether or not there was at least one OptSpec object passed in as a "linkage" in the actual arguments; and also some "chance" factor seemingly coming into play that had to with "% 2" math... :-)
But once "confused", GLM's wrapper for "GetOptionsFromArray()" would simply pass a wrong list of arguments to its wrapped GoL counterpart, causing the latter to "die" in a manner quite unexpected by module user, throwing a cryptic message difficult to comprehend.
Please note that, so far, this still does not resolve github issue [#1][proper hash-storage support]. On the other hand, it paves the way for its resolution and actually renders that fix to become quite trivial.
It would also allow and simplify revisiting the design choice mentioned in point (1) above; but that would be for an other PR, depending on how the discussion evolves.
THE NEW BEHAVIOUR
The new behavior is actually a simplification, both functionally and code-wise.
Basically, we do away with the reliance on the above mentioned assumption of ( spec => "linkage") argument pairs.
We don't even try to do any "smart" distinction between "specs" and "linkages" received as arguments, because that would either require reproducing a hunk of GoL code in the wrapper, or else prohibit stringified
Instead, we just limit ourselves to dealing with any encountered OptSpec objects themselves, passing anything else untouched to our wrapped GoL counterpart, who becomes re-enabled to do its own DRY & DWIM.
The HOW:
Added a few test cases to cover this fix (for gh #3) as well as the next one coming up that handles gh #1.
Some were already passing, some others had to be marked as "TODO" to prevent any adverse impact on current builds on Master.
The actual changes to the code were limited to two loops within the wrapper routine for "GetOptionsFromArray". Both loops had to be modified to get rid of the "% 2" assumption described above.
Here are broad locations of these loops:
Unfortunately, a surgical modification of the first loop was not quite feasible, so I had to resort to some code reorganisation, paying close attention to preserving all the intended behaviour. As a slight bonus, we now have less code in there.
The impact on performance was not measured, but I would not expect any adverse impact in that department; if anything, a very slight (minute, rather) gain can be expected.
Modifying the latter loop (2) was quite simple, just got rid of the ($i % 2) expression, replacing it with a less error prone $i > 0 enough to protect against an incorrect array access.
CLOSING REMARKS
My apologies for the somewhat lengthy message :-)
Fixes: [#3][implicit handlers/linkages] Unblocks: [#1][proper hash-storage support]
...