Skip to content
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

Fixes examples in README.md file #496

Merged
merged 1 commit into from
Apr 19, 2021
Merged

Conversation

lfrancke
Copy link
Contributor

Adapts readme to 0.52 (and probably earlier versions as well)

@@ -104,9 +104,9 @@ let watcher = watcher(api, ListParams::default());
This now gives a continual stream of events and you do not need to care about the watch having to restart, or connections dropping.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really your fault, but this is pretty misleading when the example code below fails early on a watch error (using ?). Ideally it should just log the error and move on.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, good point. We don't technically have to care about the watcher errors, but we can't bubble them up with ? either, which puts the onus of handling error cases - that looks handled from watcher internals - on the user.

Maybe we should have an interface that doesn't bubble up errors? Possibly a bit in dragon-territory because we don't want to end up error-spamming internally if misconfigured, but it might be worth doing. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to admit that I didn't even read the text :( I just tried to make the code compile.
I'm away from the computer most of the week and can't improve this at the moment. Feel free to change my PR though!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say that's definitely in dragon territory, since then we also need to provide things like a backoff policy, and more or less make the library unusable without the application using the same logging strategy as the library..

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing we /could/ do would be to add a Stream helper for these kinds of things..

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I wasn't thinking this would be mandatory, this feels like helper territory. I'll make a comment on #296 to continue this there.

Thanks @lfrancke ! Yeah, at a certain point I gloss over our text and markdown a bit myself (hence this type of state).

@clux
Copy link
Member

clux commented Apr 19, 2021

Thanks for this. Wish there was a way to have readme.md on ci via cargo-doc for this but loos like not.

@clux clux merged commit 320f6b0 into kube-rs:master Apr 19, 2021
@lfrancke lfrancke deleted the readme_fixes branch April 19, 2021 09:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants