maidsafe / self_encryption

file self encryptor
Other
118 stars 70 forks source link

refactor/all: use `Result`s throughout #185

Closed Fraser999 closed 8 years ago

Fraser999 commented 8 years ago

This changes all the public functions which can have errors to return Results. Also improved documentation and removed two unneeded tests.

For the reviewer: I split the changes into two commits to make the review easier. The first commit only contains the changes around splitting lib.rs up - there were no logic changes there at all. So, really only the second commit is interesting from a reviewer's perspective I think.


This change is Reviewable

maidsafe-highfive commented 8 years ago

r? @maqi

(maidsafe_highfive has picked a reviewer for you, use r? to override)

Viv-Rajkumar commented 8 years ago

r? @ustulation

Fraser999 commented 8 years ago

r? @dirvine

dirvine commented 8 years ago

Reviewed 3 of 12 files at r1, 2 of 4 files at r2, 1 of 1 files at r3. Review status: 5 of 12 files reviewed at latest revision, 1 unresolved discussion.


_src/data_map.rs, line 44 [r3] (raw file):_

        let mut ret = String::new();
        for byte in input_ref.iter() {
            write!(ret, "{:02x}", byte).expect("");

Can you please put descriptive text in expect calls, Just helps debug / error reporting


_Comments from Reviewable_

dirvine commented 8 years ago

Reviewed 5 of 12 files at r1, 2 of 4 files at r2. Review status: all files reviewed at latest revision, 1 unresolved discussion.


_Comments from Reviewable_

Fraser999 commented 8 years ago

Review status: all files reviewed at latest revision, 1 unresolved discussion.


_src/data_map.rs, line 44 [r3] (raw file):_

Previously, dirvine (David Irvine) wrote… > Can you please put descriptive text in `expect` calls, Just helps debug / error reporting >

Do you want unique messages, or can I duplicate all for a given function e.g. all write()s become write(...).expect("Should have written")?


_Comments from Reviewable_

dirvine commented 8 years ago

Review status: all files reviewed at latest revision, 1 unresolved discussion.


_src/data_map.rs, line 44 [r3] (raw file):_

Previously, Fraser999 (Fraser Hutchison) wrote… > Do you want unique messages, or can I duplicate all for a given function e.g. all `write()`s become `write(...).expect("Should have written")`? >

As unique as possible as these will print out on failure. So an indication of where it is, like "shoudl have written in seq" etc.


_Comments from Reviewable_

Fraser999 commented 8 years ago

Review status: all files reviewed at latest revision, 1 unresolved discussion.


_src/data_map.rs, line 44 [r3] (raw file):_

Previously, dirvine (David Irvine) wrote… > As unique as possible as these will print out on failure. So an indication of where it is, like "shoudl have written in seq" etc. >

OK. This will take me a wee while, since there are 163 occurrences (all in tests/examples/benchmarks except this debug printout one which can't fire anyway).


_Comments from Reviewable_

dirvine commented 8 years ago

Review status: all files reviewed at latest revision, 1 unresolved discussion.


_src/data_map.rs, line 44 [r3] (raw file):_

Previously, Fraser999 (Fraser Hutchison) wrote… > OK. This will take me a wee while, since there are 163 occurrences (all in tests/examples/benchmarks except this debug printout one which can't fire anyway). >

In tests that should be small then use unwrap() if it simpler (unit tests only I would say - so small simple tests where this is called only once), the [point is these being blank is then they are just unwrap() filled in with unique string will tell where they failed.


_Comments from Reviewable_

Viv-Rajkumar commented 8 years ago

Reviewed 5 of 12 files at r1, 1 of 4 files at r2, 7 of 7 files at r4. Review status: all files reviewed at latest revision, 1 unresolved discussion.


_Comments from Reviewable_

dirvine commented 8 years ago

Reviewed 1 of 4 files at r2, 7 of 7 files at r4. Review status: all files reviewed at latest revision, 1 unresolved discussion.


_Comments from Reviewable_