Reorganize kvp so that KeyValuePairs is an alias for BTreeMap instead of newtyping it #889
Conversation
|
|
||
| /// Constructs a `secrets.stackable.tech/class` annotation. | ||
| pub fn secret_class(secret_class: &str) -> Result<Annotation, AnnotationError> { | ||
| Annotation::try_from(("secrets.stackable.tech/class", secret_class)) |
There was a problem hiding this comment.
note: I think it might be useful to move these well-known keys into (non pub) constants in this module. This groups these keys in a single place.
There was a problem hiding this comment.
You can take a look at how it is done in the label module.
There was a problem hiding this comment.
I'd argue the opposite, moving towards dropping the consts module entirely since you should be using the typed interface instead. It feels weird to have both a typed and stringly interface for the same thing.
There was a problem hiding this comment.
Especially when the label values are all hardcoded in the doccomments anyway.
| impl From<&Key> for String { | ||
| fn from(value: &Key) -> Self { | ||
| value.to_string() | ||
| } | ||
| } |
There was a problem hiding this comment.
question: I'm curious why this is needed? Shouldn't we be able to call to_string() on &Key anyway?
There was a problem hiding this comment.
It makes using it in a SNAFU error slightly nicer (.context(BlahSnafu { key }) instead of .with_context(|_| BlahSnafu { key: key.to_string() })), but.. arguably the SNAFU error should just take a Key instead to begin with.
| pub mod consts; | ||
| mod key; | ||
| mod label; | ||
| pub mod label; |
There was a problem hiding this comment.
suggestion: Let's group public modules with each other. To be more specific, let's move this module declaration to the annotation one above.
| #[cfg(doc)] | ||
| use k8s_openapi::apimachinery::pkg::apis::meta::v1::ObjectMeta; | ||
| #[cfg(doc)] | ||
| use std::ops::Deref; |
There was a problem hiding this comment.
suggestion: Move these imports to the std library and external crate imports at the top of the file.
There was a problem hiding this comment.
I think it makes sense to call out doc-only "fake" imports as different from regular one (just like publics), though we definitely haven't done a great job at consistency around that in general.
| - `KeyValuePairs::insert(&mut self, kvp)`: use `::extend(&mut self, [kvp])` instead. | ||
| - `KeyValuePairs::try_from`: you may need to use `::try_from_iter` instead. | ||
| - `KeyValuePairs::contains_key`: unvalidated keys will need to use `::contains_str_key` instead. | ||
| - `Into<BTreeMap<String, String>>`: use `::to_unvalidated` instead. |
There was a problem hiding this comment.
note: I can see why this name is chosen, but can maybe be a little misleading. The source is obviously valid, the result is also valid (if not mutated).
Let's discuss some alternate names, like to_btree_map for example.
There was a problem hiding this comment.
to_btree_map is confusing since it is already one. I could see to_strings or to_string_map, but I think those are too much about the "mechanical what" rather than the "what is the effect?".
Description
This is effectively an extended version of #888 (and supersedes it).
Definition of Done Checklist
Author
Reviewer
Acceptance