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

Allow unify references #154

Merged
merged 6 commits into from
May 24, 2023
Merged

Allow unify references #154

merged 6 commits into from
May 24, 2023

Conversation

spinillos
Copy link
Member

@spinillos spinillos commented May 17, 2023

When we extend an interface with another interface, the reference is inside allOf tag of open-api schema.

Knowing that, we can move these values into a properties to render the interfaces instead their inner values. For any reason, if we don't have references, exists a default empty one that we needed to skip here.

Also, the current library implementation forces to set a type and json tag to every element and its the reason of the rest of the changes in this PR.

When we set group or expandref we aren't unable to find the reference of the interfaces and we have to put the inner interface values instead: here and here.

Update: We have a known issue with new flattern pattern where we are unable to find some references when we use _#schema definition. Using _join and schema individually fixes the issue.

@spinillos spinillos marked this pull request as draft May 17, 2023 13:22
@spinillos spinillos changed the title Allo unify references Allow unify references May 17, 2023
@spinillos spinillos marked this pull request as ready for review May 23, 2023 09:04
@spinillos
Copy link
Member Author

Each datasource is generating its own DataQuery struct... I understand that its something expected.

@sdboyer
Copy link
Contributor

sdboyer commented May 24, 2023

Each datasource is generating its own DataQuery struct... I understand that its something expected.

Yes, this is expected, at least for now. We'll soon need to deal with controls that allow us to generate these as references to imported types, similar to what we do in TS.

if hasSubpath {
for i, sel := range gen.cfg.Subpath.Selectors() {
if !val.Allows(sel) {
return nil, errors.Newf(cuetil.FirstNonThemaPos(val), "subpath %q not present in schema", cue.MakePath(gen.cfg.Subpath.Selectors()[:i+1]...))
Copy link
Contributor

Choose a reason for hiding this comment

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

just an aside - i expect we'll see more of this kind of "make thema an invisible passthrough" logic in the future - both in error output like in this case, but also when we get into references and we need the special Thema helpers that enable reference declarations to be invisible to code generators

Copy link
Contributor

@sdboyer sdboyer left a comment

Choose a reason for hiding this comment

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

Shows some oddities we'll need to deal with in future issues, but i think this is good to go as it currently stands.

}

// Afoo defines model for afoo.
type Afoo struct {
External
Extfield string `json:"extfield"`
Copy link
Contributor

Choose a reason for hiding this comment

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

This is weird. But also, what i originally committed is weird. The nilcfg output here seems sanest - it just makes the type of AFoo a reference to Foo. And it seems like that's what this should be - type Afoo Foo.

Looking at it makes me think we need clarity on what references vs. unifications are supposed to mean in the context of generated Go code, so that we have a clear rule to apply in the context of each set of options here.

So, weird though this is, let's treat it as a follow-up.

@sdboyer sdboyer merged commit 4e9d6e2 into main May 24, 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