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

fix: explode with empty separator in exemplar #587

Merged
merged 1 commit into from
Nov 28, 2023

Conversation

vnkmpf
Copy link
Contributor

@vnkmpf vnkmpf commented Nov 27, 2023

Explode with empty separator errs: https://3v4l.org/RevTj
Use mb_str_split instead: https://3v4l.org/VAKbR

Explode with empty separator errs: https://3v4l.org/RevTj
Use mb_str_split instead: https://3v4l.org/VAKbR
@neenjaw
Copy link
Collaborator

neenjaw commented Nov 28, 2023

I need a bit more of an explanation. Why does CI pass using the exemplar right now:? CI Logs

@neenjaw neenjaw self-requested a review November 28, 2023 04:23
@neenjaw neenjaw added x:size/tiny Tiny amount of work x:rep/tiny Tiny amount of reputation labels Nov 28, 2023
@vnkmpf
Copy link
Contributor Author

vnkmpf commented Nov 28, 2023

Maybe I didn't express myself clearly, why I think it's a problem.

The exercise wants you to provide types, so the tests don't test the actual behavior, which is provided in the file, but only tests correct types are added.

Why I think it should be fixed is that it shows you a code that would not work. As seen in 3v4l examples.
Taken to a bit extreme example - this code would still pass tests, but doesn't "feel right".

function letters(string $word): array
{
    throw new \RuntimeException();
}

@neenjaw
Copy link
Collaborator

neenjaw commented Nov 28, 2023

Makes sense

@neenjaw neenjaw merged commit 44ea217 into exercism:main Nov 28, 2023
20 checks passed
@neenjaw neenjaw added the x:action/improve Improve existing functionality/content label Nov 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
x:action/improve Improve existing functionality/content x:rep/tiny Tiny amount of reputation x:size/tiny Tiny amount of work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants