Conversation
86667
left a comment
86667
left a comment
There was a problem hiding this comment.
I dont like this API at all.
We have 3 PUT methods exposed: put(), put_immutable() and put_mutable(). Plus I guess announce_peer() is a PUT also?
Here we add two new function put_with_info() and put_mutable_with_info(), what about the others?
This adds 2 (or 4) functions which are behaviourally identical to the existing functions apart from their return type - they even use the same internal put_inner(). The _with_info() suffix is not useful and i dont understand why we are avoiding changing the return types of the existing functions if this is being merged into a breaking change branch anyway? Appologies if im missing something obvious, this is my immediate impression
| .map(|r| r.target) | ||
| } | ||
|
|
||
| /// Same as put, but returns a [PutResult] instead of just the target Id. |
There was a problem hiding this comment.
These docs could be much better.
What is PutResult? Why use this instead of put? what is stored_at?
We know because we understand the code and wrote these new functions, but a consumer using the lib doesnt have that context. API function docs should explain why the fn exists, what its used for, etc. AI is good at writing these
| let (sender, _) = flume::bounded::<Result<Id, PutError>>(1); | ||
| let (sender, _) = flume::bounded::<Result<PutResult, PutError>>(1); | ||
| let request = | ||
| PutRequestSpecific::PutMutable(PutMutableRequestArguments::from(item, None)); |
2 participants
Adds
PutResultwith astored_at: u8field that reports how many DHT nodes acknowledged storing a value after a PUT. The existingputandput_mutablemethods are unchanged. Newput_with_infoandput_mutable_with_infovariants returnPutResultinstead of just the target Id, this means this is a non breaking, backward compatible change.