kube-rs / kopium

Kubernetes OPenapI UnMangler
Apache License 2.0
112 stars 21 forks source link

Removing or replacing backticks from description fields? #264

Closed shaneutt closed 1 week ago

shaneutt commented 1 month ago

While working on kube-rs/gateway-api-rs we noticed that when provided triple backtick characters in a field description, such as this:

                      items:
                        description: "HTTPRouteMatch defines the predicate used to
                          match requests to a given\naction. Multiple match types
                          are ANDed together, i.e. the match will\nevaluate to true
                          only if all conditions are satisfied.\n\n\nFor example,
                          the match below will match a HTTP request only if its path\nstarts
                          with `/foo` AND it contains the `version: v1` header:\n\n\n```\nmatch:\n\n\n\tpath:\n\t
                          \ value: \"/foo\"\n\theaders:\n\t- name: \"version\"\n\t
                          \ value \"v1\"\n\n\n```"

This will result in generated output like this:

/// ```
/// match:
///
///
///     path:
///       value: "/foo"
///     headers:
///     - name: "version"
///       value "v1"
///
///
/// ```

The downside of this is that this block will trigger doctests which will fail.

Given that it seems extremely unlikely that there are CRDs out there which include backticks containing valid Rust code to test the structs they are describing, would it potentially be reasonable to filter or replace these by default, or at least add an optional flag to do so? :thinking:

clux commented 1 month ago

Yeah, generating it as is is definitely not great for rustdoc.

Filtering it out could work, but my gut feel is that it's perhaps better to inject one of ignore / custom / text after the first triple-backtick as a rustdoc attribute so that the example is kept, but not doc tested.

If we can make a regex to match on pairs of triple-backticks it should be roughly the same to do a global replace on docstrings simlarly.

Imagine this can get an escape/sanitiser in an impl Container in output.rs with tests and just used by default from main.rs. Likely not too hard. Marking this as open for now.

clux commented 1 week ago

Tested this crd with main now after alex's fix and we now get this output for the linked field:

yaml:

                        description: "HTTPRouteMatch defines the predicate used to
                          match requests to a given\naction. Multiple match types
                          are ANDed together, i.e. the match will\nevaluate to true
                          only if all conditions are satisfied.\n\n\nFor example,
                          the match below will match a HTTP request only if its path\nstarts
                          with `/foo` AND it contains the `version: v1` header:\n\n\n```\nmatch:\n\n\n\tpath:\n\t
                          \ value: \"/foo\"\n\theaders:\n\t- name: \"version\"\n\t
                          \ value \"v1\"\n\n\n```"

generated:

/// HTTPRouteMatch defines the predicate used to match requests to a given
/// action. Multiple match types are ANDed together, i.e. the match will
/// evaluate to true only if all conditions are satisfied.
/// 
/// 
/// For example, the match below will match a HTTP request only if its path
/// starts with `/foo` AND it contains the `version: v1` header:
/// 
/// 
/// ```text
/// match:
/// 
/// 
///     path:
///       value: "/foo"
///     headers:
///     - name: "version"
///       value "v1"
/// 
/// 
/// ```
#[derive(Serialize, Deserialize, Clone, Debug, JsonSchema)]
pub struct HTTPRouteRulesMatches {

as you can see it has not stripped the hard tabs, and there are still some warnings when trying to run cargo doc on it (unresolved links where cargo is being optimistic about unescaped things)

/// Converts [*].members[*].name to snake_case
              ^ no item named `*` in scope

but it does at least pass cargo doc and cargo test --doc. feel free to raise further suggestions if the above becomes annoying.

in the mean time will release this as a patch release (EDIT: 0.21.1) fixing this. thanks again to @alexsnaps for the pr!