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 unions that includes enums and a default value #93

Merged
merged 4 commits into from
Apr 13, 2023

Conversation

spinillos
Copy link
Member

@spinillos spinillos commented Mar 28, 2023

We fixed the enums defaults in this PR: #87. All tests passed correctly but when we execute this code against Grafana cue files, the results isn't the expected one.

I didn't add tests because I don't know how to write the cue structure to reproduce the case from there 😞.
Edit: We could find the way to reproduce the use case πŸ˜„. Also, because the test needs nested struct defaults to pass, I need to update the code to allow these defaults.

It happens when we set an Enum and a default value:

value: #Enum | *"a"

We expecting to have the following code:

export interface Blah {
  value: (Enum | 'a');
}

export const defaultValue = Partial<Blah> = {
  value: 'a',
}

But its generating:

export interface Blah {
  value: string;
}

export const defaultValue = Partial<Blah> = {
  value: 'a',
}

Why was the reason? Because the struct that Cuetsy received is slightly different.
In our tests:

[|]  (string)
β”œβ”€β”€ [.]  (string)
β”‚   β”œβ”€β”€ (struct)
β”‚   β”œβ”€β”€ "#Enum"
β”‚   └── [ref:#Enum]  
β”‚       └── [|]  (string) @cuetsy(kind="enum")
β”‚           β”œβ”€β”€ "a"
β”‚           β”œβ”€β”€ "b"
β”‚           └── "c"
β”œβ”€β”€ "a"
└── [*]  
    └── "a"

Grafana cue:

[]  (string)
β”œβ”€β”€ [.]  (string)
β”‚   β”œβ”€β”€ (struct)
β”‚   β”œβ”€β”€ "#Enum"
β”‚   └── [ref:#Enum]
β”‚       └── [|]  (string) @cuetsy(kind="enum")
β”‚           β”œβ”€β”€ "a"
β”‚           β”œβ”€β”€ "b"
β”‚           └── "c"
└── [*]
    └── "a"

We can see that the second one doesn't have the "a" value and we weren't analyse the cue.NoOp case for this.

So the solution is detecting the Selector operator from the first value (the Enum) to avoid to confusing it with values with validators that reaches this point (Ex: uint32 & >0 | *9), and add the default value like an existing one.

@spinillos spinillos marked this pull request as draft March 28, 2023 13:08
@spinillos spinillos marked this pull request as ready for review March 28, 2023 15:18
@spinillos spinillos requested a review from sdboyer March 28, 2023 15:18
@joanlopez
Copy link

joanlopez commented Mar 28, 2023

I didn't add tests because I don't know how to write the cue structure to reproduce the case from there 😞.

Could you share the (link to the) source of both examples (Grafana one and Cuetsy) you mentioned, please? πŸ€”

It looks good πŸ‘ŒπŸ» but I'm interested now on trying to see how could we reproduce the issue within the corpus of txtar-based tests within Cuetsy.

Thanks! πŸ˜ƒ

@spinillos
Copy link
Member Author

@joanlopez

In Cuetsy is this test but mor specific these lines and in Grafana, for example this or this.

Cuetsy tests are reproduced executing generator_test.go and for grafana, you can override the cuetsy dependency in go.mod file with your local one and do make gen-cue to see the result.

Cue tests reaching this point, since Grafana ones reaches this. You can use tpv(v) function to print the schema inside cuetsy.

@spinillos spinillos merged commit 5722229 into main Apr 13, 2023
@spinillos spinillos deleted the fix-unions-with-enums-and-one-default branch April 13, 2023 08:52
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

3 participants