Small API Improvements #102
Conversation
Hoverbear
force-pushed
the
small-improvements
branch
from
931e1cd
to
214e5da
Compare
Hoverbear
force-pushed
the
small-improvements
branch
4 times, most recently
from
53e77aa
to
bafcd69
Compare
Hoverbear
force-pushed
the
small-improvements
branch
from
bafcd69
to
6b8a3fc
Compare
| if self.voters.insert(id, pr).is_some() { | ||
| panic!("insert voter {} twice", id); | ||
| if self.learners.contains_key(&id) { | ||
| Err(Error::Exists(id, "learners"))?; |
There was a problem hiding this comment.
any different to panic outside?
There was a problem hiding this comment.
The error message will be different now. Otherwise this is just moving the error up a level so these functions can be called without worrying about panic.
Hoverbear
force-pushed
the
small-improvements
branch
from
6b8a3fc
to
985592a
Compare
|
I rebased to fix a conflict. PTAL @breeswish |
|
@BusyJay PTAL |
| @@ -55,7 +55,14 @@ quick_error! { | |||
| description(err.description()) | |||
| display("protobuf error {:?}", err) | |||
| } | |||
|
|
|||
| /// The node exists, but should not. | |||
| Exists(id: u64, set: &'static str) { | |||
There was a problem hiding this comment.
Can these two errors be merged?
There was a problem hiding this comment.
Hm, maybe? I think Exists(u64, &'static str, bool) could work? I worry that there might be some semantic issues. I was already not sure about combining things like VoterCannotBecomeLearner, VoterExists, LearnerExists, and LearnerAlreadyVoter.
There was a problem hiding this comment.
I think distinguishing them may be more clear.
There was a problem hiding this comment.
I don't think so. People usually don't care about what the exact case are they, what they do is just panic. More errors lead to longer match or repeated duplicated error1 | error 2.
There was a problem hiding this comment.
@BusyJay In the case of this particular error there are definitely recoverable cases, and they are different between the errors.
If the user is lazy and wants to panic, that's fine, but we should allow our users to write good code.
There was a problem hiding this comment.
I am fine with this, like Rust IO error, it has NotFound or AlreadyExists error for a file.
LGTM
src/progress.rs
Outdated
| if !self.learners.contains_key(&id) { | ||
| Err(Error::NotExists(id, "learners"))? | ||
| } | ||
| if let Some(mut learner) = self.learners.remove(&id) { |
There was a problem hiding this comment.
Seems check in L154 is duplicated.
Moves failable functions to returning `Result<(), Error>` and adds useful error types for determining the problem. Adds an `panic` to all calling code to preserve existing behavior.
Hoverbear
force-pushed
the
small-improvements
branch
from
0788cd7
to
b0004c2
Compare
Some small improvements to the API.
impl Traitfor returning iterators in ProgressSet.panicon bad actions, but instead report an error.