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

[Defaults] reject incomplete cue.Value when generate defaults #34

Closed
wants to merge 2 commits into from

Conversation

ying-jeanne
Copy link
Collaborator

This PR is to solve ticket #30, the bottom case is not covered yet, we need to discuss whether we should cover the case.

@ying-jeanne ying-jeanne added this to In progress in Grafana Backend (DO NOT USE!) via automation Oct 4, 2021
@ying-jeanne ying-jeanne added this to the 8.2.1 milestone Oct 4, 2021
@ying-jeanne ying-jeanne changed the title [defaults] reject incomplete cue.Value when generate defaults [Defaults] reject incomplete cue.Value when generate defaults Oct 4, 2021
@ying-jeanne ying-jeanne linked an issue Oct 4, 2021 that may be closed by this pull request
@ying-jeanne
Copy link
Collaborator Author

ying-jeanne commented Oct 5, 2021

currently the case

-- cue --
ATypedList: [...int] | *[1,2] @cuetsy(kind="type")

Foo: {
  Bar: string | *"ohai"
  Baz: int | *1
  I1_TypedList: [...int] | *[int,2]
  I2_TypedList: ATypedList
} @cuetsy(kind="interface")

would through error

can't convert list element of I1_TypedList to typescript

instead of

cannot generate default values; default value is not concrete

because the way we organize tsprintField and tsPrintDefault. the default of a specified list is actually generated in tsprintField, because its' wierd incomplete kind.

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.

Seems like it should be good. But we need to expand the matrix of test cases:

  1. Check that if there's a non-concrete default in a referenced value, the struct that references it
  2. Check that the above is true in both the case where the referenced value is @cuetsy-exported, and where it's not
  3. Check that both of the above are true whether the reference is local, or if it crosses an import boundary

i think that set of combinations should work out to four paths to check. They should all be expressible in a single txtar under testdata/imports, similar to the way paths that vary in one dimension are checked in this txtar.

return ok, err
}
for iter.Next() {
isSubEleConcret, err := isCueObjConcrete(iter.Value())
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: variable naming.

This is more art than science, but a good way i've heard it described is, variable names should get more detailed as the references to them get further away from their declarations. The only references to this are within the same control block, within a few lines of code; it's preferable to use a shorter, even not-particularly-meaningful name.

@ying-jeanne ying-jeanne removed this from In progress in Grafana Backend (DO NOT USE!) Nov 22, 2021
@sdboyer sdboyer closed this Apr 25, 2022
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.

Reject incomplete defaults
2 participants