Skip to content

simple-cipher: Disallow mixed case key#491

Merged
matthewmorgan merged 1 commit intoexercism:masterfrom
tekerson:simple-cipher/add-test-for-mixed-case-key
Oct 6, 2018
Merged

simple-cipher: Disallow mixed case key#491
matthewmorgan merged 1 commit intoexercism:masterfrom
tekerson:simple-cipher/add-test-for-mixed-case-key

Conversation

@tekerson
Copy link
Copy Markdown
Contributor

@tekerson tekerson commented Oct 3, 2018

This test has already been added to the TypeScript version of this exercise. The JavaScript example implementation already passes, but I still think the test is valuable as I've seen the same error in JavaScript student submissions.

A solution that checks if the key is valid (i.e. only contains alpha ascii lower-case letters) by testing

if (key === key.toUpperCase()) { throw new Error('Bad key') }`

passes the current "throws an error with an all caps key" test but allows mixed-case keys, which should actually be invalid.

A solution that checks if the key is valid (i.e. only contains alpha
ascii lower-case letters) by testing

    if (key === key.toUpperCase()) { throw new Error('Bad key') }`

passes the current "throws an error with an all caps key" test
but allows mixed-case keys, which should actually be invalid.
@matthewmorgan
Copy link
Copy Markdown
Contributor

Thanks @tekerson !

I was wondering about this, and I see that the canonical test file for this exercise does not contain a test against mixed-case keys, but the README specifies that the key should be all lowercase:

Let's make your substitution cipher a little more fault tolerant by providing a source of randomness and ensuring that the key contains only lowercase letters.

We appreciate you adding this here, can I suggest that you add it to problem-specifications also? It would help all the tracks.

@tekerson
Copy link
Copy Markdown
Contributor Author

tekerson commented Oct 6, 2018

Good point @matthewmorgan!
I've created a PR to add the test to the canonical test file for the problem.

@tekerson tekerson deleted the simple-cipher/add-test-for-mixed-case-key branch October 6, 2018 19:03
@matthewmorgan
Copy link
Copy Markdown
Contributor

Awesome! Thanks for doing that. I'm not a maintainer on that track, but they tend to be pretty speedy about giving feedback.

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.

2 participants