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

Let s/// return according to spec. WAS: Give the result of s/// a Bool mixin. #295

Closed
wants to merge 6 commits into from

Conversation

peschwa
Copy link
Member

@peschwa peschwa commented Jul 22, 2014

This refers to RT #82108. The performance and semantic concers
raised by jnthn++ obviously still exist.

For reference: https://rt.perl.org/Public/Bug/Display.html?id=82108

@moritz
Copy link
Member

moritz commented Jul 23, 2014

I'm less than thrilled by mixing Bool into basic types in the setting.

It might "do what I mean" in this particular context, but the result is normally a string that can be passed to anywhere else, and that anywhere else might be very surprised by strings not boolifying in the normal way.

The problem must really be solved at the spec level first, not monkey-patched in rakudo.

(IMHO the same goes for tr///, but I kept quiet there, hoping it would be just a very localized thing; but I don't want the mixing-in theme to spread).

@peschwa
Copy link
Member Author

peschwa commented Jul 23, 2014

@moritz: Right, mixins in the setting aren't great. That's not (anymore) what this PR implements, though, even though I clearly forgot to mention that it isn't. Sorry about the confusion.
The spec has been update here, which this PR implements now. I force-pushed the (as of the spec-commit correct and clarified) update into the PR branch, since the ticket is still the correct one.

@peschwa peschwa changed the title Give the result of s/// a Bool mixin. Let s/// return according to spec. WAS: Give the result of s/// a Bool mixin. Jul 23, 2014
@peschwa
Copy link
Member Author

peschwa commented Aug 2, 2014

There has been a bit more discussion here, which coupled with my own re-reading of S05 has resulted in the latest changes.
I'm rather sure this is not yet completely to spec, and thus am still working on it.
Here are notable spectest-results of the current state of the branch merged against nom, running current spectest.

As per the referenced spec commit, s/// is supposed
to always return a Match object or Nil if there was
no successful match.
Some adverbs (:g, :x, :ov, :ex) are supposed to return
a Match objects that contains all submatches and has
slightly different semantics in some cases.
Behavior of m:g// and s:g/// should now rather closely
match S05, section "Capturing from repeated matches"
and the last two paragraphs of the section "Substitution".
@peschwa
Copy link
Member Author

peschwa commented Aug 7, 2014

I would consider this branch "to spec", as it is now.
The most glaring change from the current implementation in nom is, that m:g// does not automatically return a List, and thus further processing of the return value has to explicitely force list context, e.g.:

    # previously:
    for m:g/./ { .say } # prints every single character
    # as of this branch:
    for m:g/./ { .say } # prints the whole match once, as per S05 'Capturing from repeated Matches'
    for @(m:g/./) { .say } # prints every single character

Because this might break a few things in a few places (outside of Rakudo itself, I did take care of that, to the best of my knowledge) there should maybe be some kind of deprecation cycle before this becomes mainline. Obviously some tests broke as well, I have created a branch in perl6/roast locally, which I can push. It mostly contains fixes regarding the difference mentioned above.

Edit: Here is the branch mentioned above.

@peschwa
Copy link
Member Author

peschwa commented Aug 15, 2014

There's been a bit more discussion on this around here, which suggests merging of this PR should be delayed until after the list refactor pmichaud++ has taken upon himself, mainly to conserve the for m:g/$pat/ { ... } idiom.

@peschwa
Copy link
Member Author

peschwa commented Dec 25, 2014

This PR is likely obsolete, as infix:<~~> has been changed to match Lists as truthy on its RHS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants