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

Update Str.pod6 #2155

Merged
merged 1 commit into from Jul 7, 2018
Merged

Update Str.pod6 #2155

merged 1 commit into from Jul 7, 2018

Conversation

interlab
Copy link
Contributor

@interlab interlab commented Jul 7, 2018

starts-with, ends-with returns Bool

starts-with, ends-with returns Bool
@tbrowder tbrowder merged commit b45b3c2 into Raku:master Jul 7, 2018
@tbrowder
Copy link
Member

tbrowder commented Jul 7, 2018

Thanks!

Copy link
Contributor

@JJ JJ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that I wouldn't say that behavior is defined. The code does not specify what it returns. It does return a nqp::p6bool, but I think that to make it match the actual code, it might be more correct to just eliminate the return values, since they are not there in the source. Also it's tested with ok and nok, which test "truthy" values https://github.com/perl6/roast/blob/462a86ff9b61ad7c5567b78de7f7aa1e13b4cc0f/S32-str/ends-with.t, so being precise, I would say it would better to eliminate the return values and say returns "true value" if it's identical, "false value" if it's not. But that's just me being picky, and thanks anyway for drawing the attention to this error.

@interlab
Copy link
Contributor Author

interlab commented Jul 7, 2018

perl6 -e "'qwerty'.starts-with('q').WHAT.^name.say;"

# Bool

@JJ
Copy link
Contributor

JJ commented Jul 7, 2018

@interlab that's exactly what it returns, but please read what I have pointed to. The function is not defined to return Bool (so it could return something else), and it's not tested to return Bool, so it could return something else. We should be careful to document not actual behavior, but specified behavior, and in this case the specification is vague.
In practice, the definition of the functions you are documenting is not

 multi method starts-with(Str:D: Str(Cool) $needle --> Bool:D) 

but actually

proto method starts-with(|) {*}
multi method starts-with(Str:D: Cool:D $needle) {self.starts-with: $needle.Str}
multi method starts-with(Str:D: Str:D $needle) {
    nqp::p6bool(nqp::eqat(self, $needle, 0))
}

Not even the the definition of a method of Cool, which is the closest to the one you're defining, is like that. Please check it out

method starts-with(Cool:D: |c) {
    self.Str.starts-with(|c)
}

Please note that it does not specify a return Bool either.

@tbrowder
Copy link
Member

tbrowder commented Jul 7, 2018

Oops, nice catch, @JJ, I’ll see if I can revert this.

UPDATE

Okay, reverting is ugly and still leaves us with an error as @JJ noted.

@interlab, why don’t you submit another PR implementing @JJ’s suggestions.

@AlexDaniel
Copy link
Member

Don't revert, it's better than it was.

@AlexDaniel
Copy link
Member

I filed this roast ticket: Raku/roast#446

We can change the docs to say “returns a truthy value if … , and falsey value otherwise”. Or we can just wait for a test to be written :)

Technically if a test is added, it won't be part of v6.c… But maybe that's just nitpicking and we can just move on.

@AlexDaniel
Copy link
Member

See #302 for further discussion on how to document versioned stuff.

@JJ
Copy link
Contributor

JJ commented Jul 7, 2018 via email

@AlexDaniel
Copy link
Member

@JJ Change how? See this discussion. TL;DR it is fairly obvious that starts-with should return a Bool, so in this particular case it is fine to document that. The whole situation is LTA but until something is done with #302 I don't know if there's a way to do better.

@JJ
Copy link
Contributor

JJ commented Jul 7, 2018 via email

@JJ
Copy link
Contributor

JJ commented Jul 7, 2018

And... @zoffixznet just specced that in Roast, so it's actually tested that it's returning a Bool, but still the method definition does not match what's there. So maybe change it just a tiny bit to match that?

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

4 participants