Skip to content

simple-cipher: Update description.md #1653

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Apr 30, 2020
Merged

simple-cipher: Update description.md #1653

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion exercises/simple-cipher/description.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ substitution cipher a little more fault tolerant by providing a source
of randomness and ensuring that the key contains only lowercase letters.

If someone doesn't submit a key at all, generate a truly random key of
at least 100 alphanumeric characters in length.
at least 100 lowercase characters in length.
Copy link
Member

Choose a reason for hiding this comment

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

Others are welcome to weigh in, but I don't think lowercase is implied here. A truly random key would not be limited to lower case letters. Individual tracks are welcome to add test cases of their own and should not be limited to lowercase letters. alphanumeric was added during #1346.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The canonical-data.json specifically has a test for the key being only lowercase letters on line 40, so it isn't track-specific. This actually tripped me up, because I drew my random key from strings.ascii_lowercase + strings.digits in the Python track because the instructions said "alphanumeric."

I don't really care which one it is, but if the instructions don't change, then the test in connonical-data.json should be changed or removed.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed that this is a bug. I'm in favour of changing the test to alphanum, instead of changing the description back.

Copy link
Contributor

Choose a reason for hiding this comment

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

Changing the test will be rejected due to #1560, this is clearly not a bug.

Think of it like this: the canonical-data.json is, by definition, the canon. The description.md is a story about the canon. From the moment #1560 dropped no change, omission, or imperfection in the story can override the canon... the canon can only be (un)fixed where the canon is internally inconsistent or fundamentally wrong and the inputs given in a test should not produce the outputs described in that test.

There is no fundamental, non-personal-preference, reason why uppercase characters must be included — we have artificially constrained many exercises in artificial ways — but if a test exists that enforces that constraint then the canon has spoken and the description is wrong.

Copy link
Member

Choose a reason for hiding this comment

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

  1. Change the description to lowercase and this can be merged. It's the "wrong" fix but it can move forward under [NOTICE] This repo is temporarily only accepting bug-fixes. #1560.
  2. Change the test and this can't be merged ([NOTICE] This repo is temporarily only accepting bug-fixes. #1560) but it's probably the right fix.

I'd vote for (1) for now. Then do (2) once #1560 is released (which will probably be once v3 is launched and all exercises in this repo become practice exercises again)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's likely obvious from what I said before, but to be explicit I'd also vote for 1 and for an Issue to be created, on hold, for making this case-insensitive in the future.

I wish there was a way for us to simply prevent a PR from being submitted without an Issue already existing and being referenced. I hate the idea of people doing work that will end up stuck.

Copy link
Member

Choose a reason for hiding this comment

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

I hate the idea of people doing work that will end up stuck.

Me too! But it will get merged eventually.


## Extensions

Expand Down