rotational-cipher: Added negative shiftKey test cases

[ZapAnton]

New test cases ask students to handle negative shift keys, which makes the exercise a little more challenging and could increase learning.

[ErikSchierboom]

Thanks for the PR! I must admit though, that I'm not entirely sure if there is much benefit in the added test cases. Maybe someone has a different opinion?

[ZapAnton]

@ErikSchierboom for some rationale you may refer to the discussion in exercism/rust#782

[ErikSchierboom]

Thanks for the link! It does give some context on why this might be desirable. I'll hold off until more people have chipped in with their thoughts.

[ErikSchierboom]

@petertseng @rpottsoh Could I ask you for your opinions?

[rpottsoh]

I have been thinking about this and I guess I am not opposed to the additional tests. However, a negative shift is not covered as an example in README. Should description.md be updated to include a negative example?

[ZapAnton]

@rpottsoh how about these changes to the README:

• After the 'The letter is shifted for...' sentence add the following sentence:

Notice, however, that you are not restricted to the positive-only keys. The keys from `-26` to `-1` are also valid.

• Add the following example in the 'Examples' section:

ROT-1 `m` gives `l`

[ErikSchierboom]

I've given this some more thought, and I'm a bit on the fence here. I understand that the Rust track wants an exercise to introduce unsigned types, but it also makes this exercise a tiny bit more difficult for students to implement. In general, exercises defined in this repository have a tendency to become more complex, ultimately leading to less simple exercises and many complex ones.

In this particular case, I'm okay with the change (though not necessarily in favor of it), but I did want to make above remark.

[rpottsoh]

@ErikSchierboom do you think description.md should also be updated?

[ErikSchierboom]

@rpottsoh I do!

[rpottsoh]

I have been working on this exercise today for the Delphi track and included the proposed cases. I would like to propose another idea. @ErikSchierboom is correct that adding these two cases does make the exercise harder... Perhaps the new cases could be introduced as bonuses (optional)?

@ZapAnton your proposed changes for description.md are fine. We may want to place them differently in the file if my suggestion above is taken seriously.

[ZapAnton]

@rpottsoh , in that case how about moving the additional README part in the separate 'Bonus task' section? For example:

---

Bonus task

For an additional challenge, you may want to take a look at the last two test cases, rotate m by negative 1 and rotate letters by negative 26. These tests expect you solution to work with the negative cipher keys from -1 to -26, which, technically, are also valid keys. For example ROT-1 from m gives l. Note, that these test cases are optional and you are free to skip them, if you have problems with your solution.

---

This way we isolate the additional test cases from the rest of the README, which allows us to remove it in the future, if the need arises.

Edit: This also implies, that the cases in the canonical data are placed at the end, so not to confuse track maintainers too much.

[ErikSchierboom]

Edit: This also implies, that the cases in the canonical data are placed at the end, so not to confuse track maintainers too much.

I don't think this will work, as there a quite some languages that auto-generate the exercises. Whether they are listed at the end or not does not make any real difference, expect that one could theoretically exclude the last two test cases. Also, in my experience people will often not read the README after having done the first couple of exercises, and just start coding right away. The fact that the last two test cases are bonus exercises seems to me to be fairly confusing if they are included as regular test cases.

My preferred solution would be to not have these additional test cases added to the canonical data, and to have each track that wants more test cases add them manually.

@F3PiX @petertseng any thoughts here?

[ZapAnton]

Alternatively, the new test cases could be switched to a new property (for instance rotateNegative) with the comments field that would contain something along the lines of 'This is a bonus property cases. Implement them when you see fit.'

[emcoding]

My preferred solution would be to not have these additional test cases added to the canonical data, and to have each track that wants more test cases add them manually.

That sounds good to me.

[rpottsoh]

@ErikSchierboom's suggestion I think is the most sensible.

Any tracks that are interested in adding cases like these may do so. Those tracks may also choose to indicate in the test suite via comments or whatever mechanism is convenient, that these two cases are bonuses.

[ZapAnton]

@ErikSchierboom guess this PR could be closed, since the final verdict is to not add new test cases.

[ErikSchierboom]

@ZapAnton I agree. Note that I really do appreciate you taking the time to create this PR, it's just that in this particular case I'm not in favor of this specific change. That said, each track is of course free to add particular tests themselves. This is something that I've done in the C# track too, where I added some additional tests that made more sense there, and would not necessarily benefit other tracks.

Once again, thanks for the PR and I'm sorry the end result may be disappointing to you.