-
-
Notifications
You must be signed in to change notification settings - Fork 311
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
runtime: rename references from Flatten to Decode #1520
Conversation
closes #1517 Signed-off-by: clux <[email protected]>
Signed-off-by: clux <[email protected]>
Signed-off-by: clux <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1520 +/- ##
=====================================
Coverage 75.3% 75.3%
=====================================
Files 82 82
Lines 7331 7331
=====================================
Hits 5515 5515
Misses 1816 1816
|
Signed-off-by: clux <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No blockers from my side, although I did have some thoughts. Looks good and it's nice we managed to simplify the whole machinery!
@@ -9,17 +9,17 @@ use pin_project::pin_project; | |||
#[pin_project] | |||
/// Stream returned by the [`applied_objects`](super::WatchStreamExt::applied_objects) and [`touched_objects`](super::WatchStreamExt::touched_objects) method. | |||
#[must_use = "streams do nothing unless polled"] | |||
pub struct EventFlatten<St> { | |||
pub struct EventFilter<St> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpicky thought: EventFilter
sounds like a good name imo, especially given its use now. I wonder thought if it makes sense to me because I've seen it before. Would users conflate it with PredicateFilter
(although, being my own's devil advocate, that's what docs are for).
Would a name like EventDecode
work better to convey that what we are actually doing is just taking the inner value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm, yeah... possibly. maybe i'll hang on to this PR for a little bit first to see if it clashes with use cases for stream forwarding (since it was also running into the same issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have updated to EventDecode
now.
Co-authored-by: Matei David <[email protected]> Signed-off-by: Eirik A <[email protected]>
Signed-off-by: Eirik A <[email protected]>
Signed-off-by: clux <[email protected]>
Signed-off-by: clux <[email protected]>
Signed-off-by: clux <[email protected]>
closes #1517 - we no longer flatten anything.
creates a deprecated type alias for
EventFlatten
in its place because is actually exposed. Users ofWatchStreamExt
direct will not notice / care for this, but would be nice if direct users - if they exist - get a warning.docs updated in kube-rs/website#66