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

feat: improve parsing #51

Merged
merged 6 commits into from
Oct 6, 2023
Merged

feat: improve parsing #51

merged 6 commits into from
Oct 6, 2023

Conversation

tenstad
Copy link
Contributor

@tenstad tenstad commented Aug 9, 2023

Simplifies a lot of the complexity of having separate processType, loadType and loadAliasType.

Removes ArrayKind, as it is not supported by controller-gen.

Also ensures correct parsing of A in the example below, making A.UnderlyingType equal B instead of string. This will be required to correctly parse and propagate markers (#49).

type Foo struct {
	A A `json:"a,omitempty"`
}

type A B
type B string

@thbkrkr
Copy link
Contributor

thbkrkr commented Oct 5, 2023

This is very nice!

Also ensures correct parsing of A in the example below, making A.UnderlyingType equal B instead of string. This will be required to correctly parse and propagate markers (#49).

Could you update test/api/v1/guestbook_types.go with an example that covers this?

Suggestion but feel free to rename:

// UnderlyingType tests that A.UnderlyingType equals B instead of string.
// +kubebuilder:object:root=true
type UnderlyingType struct {
	A A `json:"a,omitempty"`
}

type A B
type B string

I think we need to adapt RenderTypeLink a bit in both renderers because we can now have a link in a title which isn't rendered as expected.

@cla-checker-service

This comment was marked as resolved.

@thbkrkr

This comment was marked as resolved.

@tenstad
Copy link
Contributor Author

tenstad commented Oct 6, 2023

Added the test in two commits, where the 2nd (319af4e) shows the changes introduced by this PR.

@tenstad
Copy link
Contributor Author

tenstad commented Oct 6, 2023

Wondering if we should update the rendering so that the test types we added renders as they used to 🤔

@tenstad
Copy link
Contributor Author

tenstad commented Oct 6, 2023

Having different descriptions, I guess it is nice to show both Underlying1 and Underlying2 :)

@thbkrkr
Copy link
Contributor

thbkrkr commented Oct 6, 2023

Last changes are 👍 👍 .

I think that the last bits to fix are for the rendering.

In asciidoc, it looks like xref doesn't work well when used in a title. See the example below where the cross reference for Underlying2 doesn't work:

Capture d’écran 2023-10-06 à 10 24 51

The only idea I have for now is to use link:#, which seems a bit hacky?

For markdown, we need to add the backquotes conditionally otherwise the link is not rendered:

Capture d’écran 2023-10-06 à 11 57 31

@tenstad
Copy link
Contributor Author

tenstad commented Oct 6, 2023

What if we display Underlying type: in both adoc and markdown. And show the type as in a struct field?

Copy link
Contributor

@thbkrkr thbkrkr left a comment

Choose a reason for hiding this comment

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

LGTM!

We really appreciate your contribution, thanks again.

@thbkrkr thbkrkr merged commit 365ecbe into elastic:master Oct 6, 2023
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.

2 participants