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

fix #1772: EncodeType doesn't unify embedded Go struct with struct definition #1775

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

yangzhares
Copy link

Signed-off-by: Clare Yang (zhanyang) zhanyang@cisco.com

@yangzhares yangzhares requested a review from cueckoo as a code owner June 25, 2022 08:14
@myitcv
Copy link
Member

myitcv commented Jun 27, 2022

Thanks @yangzhares - I'll defer to @mpvl for a review here.

In the meantime, please can you take a look at the contribution guide on commit messages?

https://github.com/cue-lang/cue/blob/master/doc/contribute.md#good-commit-messages

That gives advice on the scheme we follow there. Thanks

@verdverm
Copy link

I'm curious what happens if the embedded struct does not have a json tag. A test covering this would be nice too, if it doesn't already exist somewhere

…nition

This actually fixes a bug in GoTypeToExpr under internal/core/convert, it
doesn't unify embedded Go struct with struct definition, and add two test
cases to verify this fix working well.

Fixes: cue-lang#1772

Signed-off-by: Clare Yang (zhanyang) <zhanyang@cisco.com>
@yangzhares
Copy link
Author

yangzhares commented Jun 28, 2022

@myitcv , updated commit message.

@verdverm , added two test cases, i'm not sure that is what you want

@verdverm
Copy link

@yangzhares That looks to cover what I was thinking about, though now I'm wondering what happens with E.

type EmbeddedObj struct {
	B int `json:"b"`
	D string
	E string `json:"e",omitempty`
}

@yangzhares
Copy link
Author

yangzhares commented Jun 28, 2022

Based on current logic 1) goTypeToValueRec, https://github.com/cue-lang/cue/blob/master/internal/core/convert/go.go#L693, 2) getName, https://github.com/cue-lang/cue/blob/master/internal/core/convert/go.go#L99, 3)isOptional, https://github.com/cue-lang/cue/blob/master/internal/core/convert/go.go#L139, to handle such case, especial field E, its CUE code should be like:

{
    b: ((int & >=-9223372036854775808) & <=9223372036854775807)
    D: string
    e: string
}

@verdverm
Copy link

I'm not familiar with the code, mostly just thinking about test cases to cover all the scenarios, especially when code changes in the future

@mvdan
Copy link
Member

mvdan commented Apr 28, 2023

@yangzhares apologies that we hadn't gotten back to this yet. It appears that other fixes have caused conflicts here. Would you be able to fix those and rebase? I'm happy to review afterwards. Thank you!

@mvdan mvdan assigned mvdan and unassigned mpvl Apr 28, 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.

None yet

5 participants