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

Add prelude for blanket and extension traits across sub-crates #1527

Merged
merged 1 commit into from
Jul 15, 2024

Conversation

Danil-Grigorev
Copy link
Member

@Danil-Grigorev Danil-Grigorev commented Jun 20, 2024

Motivation

Slight follow up on proposed changes in #1511 (comment)

Solution

Include prelude into kube crate, exporting all blanket implementations, such as:

  • client::ConfigExt - client configuration options.
  • client::scope::NamespacedRef - namespace scoped resource fetching via client.fetch (under unstable-client feature).
  • core::PartialObjectMetaExt - useful for cache size reduction in cases when only watched object type and metadata are required.
  • core::crd::CustomResourceExt - providing methods such as .crd() for custom resource generation.
  • core::crd::{Resource, ResourceExt} - base and extension functionality on top of any valid kubernetes resource.
  • runtime::utils::WatchStreamExt - utility methods for watcher streams, providing predicate capabilities, stream sharing with reflect and reflect_shared (under runtime feature)

@Danil-Grigorev Danil-Grigorev changed the title [WIP] Add basic prelude for client lib [WIP] client: Add basic prelude for client lib Jun 20, 2024
Copy link
Member

@clux clux left a comment

Choose a reason for hiding this comment

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

Thanks for starting this, I was about to write an issue for this!

Couple of things of-the-top of my head that could be useful:

//!
//! The prelude may grow over time as additional items see ubiquitous use.

#[cfg(feature = "unstable-client")] pub use crate::client::scope::*;
Copy link
Member

@clux clux Jun 20, 2024

Choose a reason for hiding this comment

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

I see people are importing traits in prelude using pub use ExplicitScope as _; to presumably avoid warnings
https://github.com/tokio-rs/tracing/blob/master/tracing-subscriber/src/prelude.rs

ideally people should not need to opt-in to unused_imports

Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure adding feature gates in a prelude is a good idea? People doing a wildcard import on prelude crate may get their symbols conditionally shadowed if another crate enabled this feature.

Copy link
Member

Choose a reason for hiding this comment

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

I think the shadowing is a bigger problem than the feature gates, and the shadowing can be avoided by use X as _;. It limits the prelude to extension trait stuff, but maybe that is a better way forward?

Copy link
Member Author

Choose a reason for hiding this comment

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

So as an update - I added all blanket implemented traits and extension traits to the prelude, while keeping those under feature gates separately - as prelude_unstable. I'm not sure that 2 preludes are needed, but went with separate so far. Also removed all struct, to avoid opt-in for unused imports. It would be perfect to have an option to say - prelude can be used but not re-exported (as in lib.rs), but I don't know if it is possible 🙂

@Danil-Grigorev Danil-Grigorev force-pushed the unstable-fetch-prelude branch 4 times, most recently from 347e05b to e8ed920 Compare June 23, 2024 11:28
Copy link

codecov bot commented Jun 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.2%. Comparing base (7a10e24) to head (a2527ef).

Additional details and impacted files
@@          Coverage Diff          @@
##            main   #1527   +/-   ##
=====================================
  Coverage   75.2%   75.2%           
=====================================
  Files         78      78           
  Lines       6989    6989           
=====================================
  Hits        5249    5249           
  Misses      1740    1740           
Files Coverage Δ
kube-client/src/client/client_ext.rs 84.5% <ø> (ø)
kube-runtime/src/utils/watch_ext.rs 22.3% <ø> (ø)
kube/src/lib.rs 88.5% <ø> (ø)

@Danil-Grigorev Danil-Grigorev force-pushed the unstable-fetch-prelude branch 3 times, most recently from eecd7b5 to 1136755 Compare June 23, 2024 14:13
@Danil-Grigorev Danil-Grigorev changed the title [WIP] client: Add basic prelude for client lib client: Add basic prelude for client lib Jun 23, 2024
@Danil-Grigorev Danil-Grigorev changed the title client: Add basic prelude for client lib client: Add basic prelude for client, core and runtime lib Jun 23, 2024
Copy link
Member

@clux clux left a comment

Choose a reason for hiding this comment

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

Some concerns.

kube-client/src/lib.rs Outdated Show resolved Hide resolved
kube-core/src/lib.rs Outdated Show resolved Hide resolved
kube-core/src/lib.rs Outdated Show resolved Hide resolved
kube-runtime/src/lib.rs Outdated Show resolved Hide resolved
kube-runtime/src/lib.rs Outdated Show resolved Hide resolved
@Danil-Grigorev Danil-Grigorev changed the title client: Add basic prelude for client, core and runtime lib [WIP] client: Add basic prelude for client, core and runtime lib Jun 24, 2024
@Danil-Grigorev Danil-Grigorev force-pushed the unstable-fetch-prelude branch 2 times, most recently from e223729 to ee5a2f5 Compare July 11, 2024 15:45
@Danil-Grigorev Danil-Grigorev requested review from clux and SOF3 July 11, 2024 15:46
@Danil-Grigorev Danil-Grigorev changed the title [WIP] client: Add basic prelude for client, core and runtime lib Add prelude for blanket traits across sub-crates Jul 11, 2024
@Danil-Grigorev
Copy link
Member Author

@clux Should be fixed now, I removed everything causing a concern, leaving only subset of trait extensions. Also moved it to kube crate.

Copy link
Member

@clux clux left a comment

Choose a reason for hiding this comment

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

I think this looks more appropriate. Two lines that can be avoided, but the exports there now are mostly sensible to me.

kube/src/lib.rs Outdated Show resolved Hide resolved
kube/src/lib.rs Outdated Show resolved Hide resolved
@clux clux added the changelog-add changelog added category for prs label Jul 12, 2024
@clux clux added this to the 0.93.0 milestone Jul 12, 2024
@Danil-Grigorev Danil-Grigorev changed the title Add prelude for blanket traits across sub-crates Add prelude for blanket and extension traits across sub-crates Jul 12, 2024
@Danil-Grigorev
Copy link
Member Author

Added some better description, JIC someone returns to this. Thanks for the review @clux

@clux clux merged commit e57b060 into kube-rs:main Jul 15, 2024
17 checks passed
@clux clux mentioned this pull request Jul 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog-add changelog added category for prs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants