Skip to content

two-fer: Add test for handling empty string param#2107

Closed
nguyendat wants to merge 1 commit intoexercism:masterfrom
nguyendat:master
Closed

two-fer: Add test for handling empty string param#2107
nguyendat wants to merge 1 commit intoexercism:masterfrom
nguyendat:master

Conversation

@nguyendat
Copy link
Copy Markdown

@nguyendat nguyendat requested a review from a team as a code owner November 1, 2019 09:30
Copy link
Copy Markdown
Contributor

@cmccandless cmccandless left a comment

Choose a reason for hiding this comment

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

Test cases for this exercise are defined in the canonical-data. For more information on the structure of Exercism track repositories, please see the Contributing Guide

@yawpitch
Copy link
Copy Markdown
Contributor

yawpitch commented Nov 5, 2019

@nguyendat I'm afraid this test case doesn't really make sense, and neither does the discussion on that exercise: the purpose of a default value is to be used only if no argument whatsoever is supplied for that parameter ... if any string-convertible argument is provided the argument should be used, even if it's nonsensical, otherwise you're second guessing your user's intent, and the user is allowed to have nonsensical intent.

So the "correct" response for two_fer("") really is "One for , one for me." ... it might not make the most sense, but it's more consistent than assuming that an explicitly passed empty string is the same thing as no string being passed at all. Similarly two_fer(None) really should return "One for None, one for me." ... if the user explicitly passes an argument we should tend to assume that was an informed choice, even if it's a silly one.

I'm going to close this as wontfix for that reason, but thank you for the contribution.

@yawpitch yawpitch closed this Nov 5, 2019
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.

3 participants