arosspope / cipher-crypt

A cryptographic tomb of ciphers forgotten by time.
Apache License 2.0
42 stars 8 forks source link

Add Result returns for cipher traits #9

Closed dex-r closed 7 years ago

dex-r commented 7 years ago

I've added result return types for both the encrypt and decrypt traits, and have added unwraps everywhere in the tests and docstrings. So many unwraps. Currently, none of the ciphers actually return an Err.

arosspope commented 7 years ago

Great work. However, there are a few places in the current code where I would like to be explicitly returning an Err.

dex-r commented 7 years ago

So I've added the return types as requested, which has resulted in the ROT13 cipher needing a Result return type as well, which i guess makes it consistant with the other ciphers. However it means that the example in the README is a bit cluttered with unwraps, so if you agree i think we should split that into multiple lines. I've also snuck in a fix for the exhaustive_encrypt() test in caeser.rs which wasnt testing the last shift. It seemed too small for its own issue.

arosspope commented 7 years ago

Thanks for those changes, however I dont think the rot13 apply function should need to return a Result type. It doesnt implement the cipher trait, and there is no case where it would fail.

If you have a strong opinion I'd be happy to discuss but otherwise - I dont think it needs to have it.

dex-r commented 7 years ago

Its only that it uses the shift_substitution() in common, that now returns a Result. I agree that it shouldn't return an Err though. Should I change it to unwrap automatically?

arosspope commented 7 years ago

Yeah, perhaps it makes more sense to just let it panic if something goes wrong. Perhaps just add a comment for our reasoning