affine-cipher: Add exercise #897
Conversation
ktomsic
force-pushed
the
affine-cipher
branch
from
c0997c0 to
4e4615e
Compare
There was a problem hiding this comment.
Thanks @ktomsic for this work! I have some notes, below, but none are serious enough to preclude approval.
It would be convenient for me as a maintainer if you were willing to wait for the resolution of exercism/problem-specifications#1614 and then update this PR accordingly. However, if you'd prefer not to wait, I can understand that. Just let me know your intention.
My intention, if you want to get this merged ASAP and nobody objects, is to merge this on Monday. If I don't hear back from you, I will assume you are willing to wait for the problem specifications change and incorporate it into this PR.
| @@ -0,0 +1,18 @@ | |||
| /// While the problem description indicates a return status of 1 should be returned on errors, | |||
| /// it is much more common to return a `Result`, so we provide an error type for the result here. | |||
There was a problem hiding this comment.
Your reasoning here is precisely correct, but you should be aware of exercism/problem-specifications#1614. At your discretion, you can choose to wait for that issue to be resolved before moving forward with this PR; just let me know. If you'd prefer to get this merged as quickly as possible, that is acceptable.
There was a problem hiding this comment.
I can wait, and I'll update the phrasing when the specification's description.md gets updated.
| fn encode_with_a_not_coprime_to_m() { | ||
| let expected_error = AffineCipherError::NotCoprime(6); | ||
| match encode("This is a test.", 6, 17) { | ||
| Err(AffineCipherError::NotCoprime(6)) => (), |
There was a problem hiding this comment.
| Err(AffineCipherError::NotCoprime(6)) => (), | |
| Err(expected_error) => (), |
Note that this will require #[derive(Clone,Copy)] on AffineCipherError.
There was a problem hiding this comment.
The suggested approach doesn't quite work -- it binds a local expected_error variable to whatever Err() contains rather than matching against the outer expected_error -- but it did get me thinking about this a little differently.
While we can't pattern match on variables, we can pattern match on constants (provided they implement Eq and PartialEq), so I changed expected_error into a const and added the appropriate derives.
Hopefully that's a little bit closer to what you're looking for.
There was a problem hiding this comment.
Yes, it is; that looks great! Sorry for having lead you astray with the variable binding.
ktomsic
force-pushed
the
affine-cipher
branch
from
4e4615e to
9ec5a0a
Compare
|
|
||
| #[test] | ||
| fn encode_yes() { | ||
| assert_eq!(encode("yes", 5, 7).unwrap(), "xbt") |
There was a problem hiding this comment.
There was once upon a time in #338 where we thought all assertions of the form assert_eq!(a.unwrap(), b) should instead be written as assert_eq(a, Ok(b)) because it gave better test outputs when the implementation said Err when it should instead give Ok.
However, I have just given this a try, and here is a comparison of the two:
---- tests::dont_unwrap_and_expect_ok_string stdout ----
thread 'tests::dont_unwrap_and_expect_ok_string' panicked at 'assertion failed: `(left == right)`
left: `Err(NotCoprime(2))`,
right: `Ok("hello")`', src/lib.rs:16:9
---- tests::do_unwrap_and_expect_string stdout ----
thread 'tests::do_unwrap_and_expect_string' panicked at 'called `Result::unwrap()` on an `Err` value: NotCoprime(2)', src/libcore/result.rs:1084:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
I deem this sufficiently helpful (shows what was attempted to be unwrapped) that we no longer need to consider #338. In fact this has always been the case (it had always shown the Err that was unwrapped, even at the time of filing #338), so it's simply that this track has generally gotten better at having good Errors, and this caused me to change my mind on the issue.
I think the former is still slightly more helpful (shows what was actually expected) and should be used in situations where it is possible without inconvenience, but note that since this is a Result<String, AffineCipherError>, we don't seem to just be able to write Ok("xbt"), but instead would have to write Ok("xbt".to_string()) or something. I deem that enough of an inconvenience so as to say it need not happen here. I leave it as a comment in case anyone feels differently.
| #[ignore] | ||
| fn decode_with_a_not_coprime_to_m() { | ||
| const EXPECTED_ERROR: AffineCipherError = AffineCipherError::NotCoprime(13); | ||
| match decode("Test", 13, 11) { |
There was a problem hiding this comment.
although the canonical data has 5 in place of 11 here, since this is meant to error anyway, I have no reason to prefer one value over the other. So I accept this difference.
3 participants
This adds the affine-cipher exercise from the problem specifications repo.