Add Api::get_opt for better existence handling
#809
Conversation
Fixes kube-rs#806 Signed-off-by: Teo Klestrup Röijezon <teo@nullable.se>
Codecov Report
@@ Coverage Diff @@
## master #809 +/- ##
=======================================
Coverage 70.04% 70.05%
=======================================
Files 58 58
Lines 3973 3984 +11
=======================================
+ Hits 2783 2791 +8
- Misses 1190 1193 +3
Continue to review full report at Codecov.
|
kube-client/src/api/core_methods.rs
Outdated
| pub async fn try_get(&self, name: &str) -> Result<Option<K>> { | ||
| match self.get(name).await { | ||
| Ok(obj) => Ok(Some(obj)), | ||
| Err(Error::Api(ErrorResponse { reason, .. })) if &reason == "NotFound" => Ok(None), | ||
| Err(err) => Err(err), | ||
| } |
There was a problem hiding this comment.
I think the logic and tests for this are all good. I am only on the fence with the name here as it's a bit clashing with the normal expectations of the try_ prefix. The other alternatives; get_optional and get_opt does feel a bit more awkward though.
I kind of feel like this method should be made into .get as that is more rust-standard, and maybe relegate the old getter to .get_raw or .get_unchecked, even if this is a breaking change.
There was a problem hiding this comment.
I agree that this is awkward, but if we change .get to special case 404, isn't that also awkward as an abstraction for HTTP GET?
I can't think of a good name either, but we might want to avoid try_ if possible.
There was a problem hiding this comment.
Yeah, upon reflection, it's probably not wise to change .get. A get_or_none works, although it kind of implies the existence with get_or which feels more like an Entry thing.
Maybe .get_ok? Result::ok converts from Result to Option, which is similar (but not quite the same), but perhaps signals an option here more than the result?
There was a problem hiding this comment.
Personally, I don't like get_or_none or get_ok since they imply a mechanical transformation rather than what conditions we're separating out.
There was a problem hiding this comment.
True, although those functions also return Options so the signature is at least consistent.
There was a problem hiding this comment.
As does slice::get. We're a relative outlier in not using Option consistently. They use x[i] for "asserting get", which we might technically be able to get away with too, but I think it would end up pretty awkward in terms of both usage and implementation.
There was a problem hiding this comment.
FWIW, to my Rust taste find feels like a perfect match for intended behavior
There was a problem hiding this comment.
The signature for find technically matches, but behaviourally feels weird to me as well. It's a lookup on name, not a predicate search. Would personally prefer to stay within the conventions of db/hash table lookups with some variant of .get* (this also makes it appear right next to .get in docs.rs - which i think also help alleviate confusions).
Out of all of these suggestions, and given there are legit concerns here with every other suggestion (mismatching expectations / long names / convention breaks), maybe we should reconsider pivoting back onto get_opt and fix what it means more with documentation. E.g. add a line to the original get to cross-link:
/// NB: this returns an error type when the named object does note exist, if you want to catch this error more easily, consider [`Api::get_opt`].
There was a problem hiding this comment.
Okay, renamed it to get_opt and added a docs note to get
In accordance with feedback in the PR. Signed-off-by: Teo Klestrup Röijezon <teo@nullable.se>
clux
changed the title
Api::try_getApi::get_opt for better existence handling
Motivation
Fixes #806
Solution
Adds a new method
Api::try_get, as described in #806 (calledget_optthere).I think an
EntryAPI is still worthwhile, but that still requires this as groundwork.