-
Notifications
You must be signed in to change notification settings - Fork 8
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
[Ready] Remove deprecated functions #28
Conversation
@@ -456,7 +456,7 @@ func (g *generator) genInterface(name string, v cue.Value) { | |||
// place in the body of the generated typescript interface. (Or nil, if | |||
// there's no body to generate.) | |||
for fields != nil && fields.Next() { | |||
if fields.IsHidden() { | |||
if fields.Selector().PkgPath() != "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this the new canonical way to tell if a field is hidden? It seems like this is checking something else
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked the code comments.
// IsHidden reports if a field is hidden from the data model.
//
// Deprecated: use i.Selector().PkgPath() != ""
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the test it seems behaving the same, https://github.com/ying-jeanne/cue_test_cases/blob/d4c23b79200cc30d5a102b9ee90ad197e03da218/test2.go.
When fields are hidden, there pkgpath is _ instead of empty
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha, thanks for the thoroughness! i'll chalk this up to some kinda continued awkwardness in the CUE APIs around hidden fields
@@ -40,8 +41,8 @@ func TestGenerate(t *testing.T) { | |||
|
|||
for _, c := range cases { | |||
t.Run(c.Name, func(t *testing.T) { | |||
var r cue.Runtime | |||
i, err := r.Compile(c.Name+".cue", c.CUE) | |||
ctx := cuecontext.New() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm... no change necessary for this PR, but i wonder if public cuetsy functions should start taking a context argument and using it to decide things like how far we dereference a structure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds nice, :) just saw that we can pass the scope parameter, probably able to do it that way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
created a new ticket #29
This is to fix issue #25