cue: add function for Value.Attributes() #529

verdverm · 2020-09-17T19:27:54Z

This enables a user of the Go API to get all attributes so they can inspect them, rather than only being able to ask if a particular attribute exists on a value. Really useful for tools leveraging CUE!

I recall @mpvl might have wanted the return to be a Value or List rather than the slice?

cue/types.go

mpvl · 2020-09-30T13:47:51Z

cue/types.go

@@ -2007,11 +2007,48 @@ func (v Value) Attribute(key string) Attribute {
 	return Attribute{internal.NewNonExisting(key)}
 }

+// Attributes returne all attributes for the Value.


s/returne/reports/

Note that the old API is a bit broken in that there are now both field attributes and attribute declarations. But that is not the problem of this CL. The attribute declarations could perhaps be obtained through Instance and Struct.

Either way, will be good to be explicit about what this does exactly in the doc comment.

changed to "Attribute reports all field attributes for the Value"

internal/attrs.go

cue/types.go

mpvl · 2020-09-30T13:54:27Z

cue/types.go

+	a.attr.SetName(name)
+}
+
+func (a *Attribute) Vals() map[string]string {


I would call this Values or Map, in general Go style is not in favor of using contractions.

But I would remove this method. It is not necessary, as everything can be done without this method and this method exposes implementation details in the API. It also papers over the fact that there can be multiple attributes with the same key in a field.

I have this because I'd like to grab all the key/value items in an attribute, and then iterate over those making decisions along the way. I'm not sure how to iterate over all the fields in an Attribute without a function to access them. Maybe there should be a function Fields to return them all as a slice instead of this? Renamed this to Map for now.

@mpvl I'm confused by your last statement, could you provide an example of what you mean by "multiple attributes with the same key in a field?"

cue/types.go

googlebot · 2020-10-05T03:42:45Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

googlebot · 2020-10-05T03:42:46Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

verdverm · 2020-10-05T03:44:14Z

@mpvl did I mess this PR up with a rebase?

I addressed the comments you made but now I see a ton of other commits...

(edit) think I fixed the rebase issue 🤦

googlebot · 2020-10-05T03:46:32Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

googlebot · 2020-10-05T03:46:32Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

verdverm · 2020-10-19T18:23:20Z

I've found the map to be less ideal, preferring to have the written order maintained, like Cue does elsewhere.

Now thinking a Attrs.Values() cue.Value with an underlying struct might make more sense.

Thoughts?

mpvl · 2021-01-27T05:51:16Z

Note that there are now also attribute at the declaration level. So the API needs to take these into account as well.

verdverm · 2021-01-27T06:10:46Z

tbh, the Attribute API is the more restrictive one for me than the Value.Attribute() vs getting all of them.

Are they both using the same underlying Attribute type?

I'd like to be able to extract all of the content from within the Attribute () without having to parse it myself

mpvl · 2021-01-28T19:06:25Z

I've added a function to get declaration attributes. This may help in coming up with an API that supports both.

https://cue-review.googlesource.com/c/cue/+/8342

verdverm · 2021-02-17T19:35:31Z

@mpvl this seems to start supporting the suggestion you made here: #774 (comment)

I have a number of open issues / requests / etc around Attributes, maybe a proposal of sorts is in order for them? I can start a new issue outlining all of these, something like "Attribute(s) API"

thoughts?

mpvl · 2021-02-18T15:40:03Z

@verdverm : I've started converging on what would be a proper API, so maybe we don't need to go through these lengths.

In a nutshell, we ideally should have only one call for fetching attributes (Value.Attributes or Value.Attrs) that returns all attributes associated with a value, so field and declaration attributes, but then annotate the kind in the Attribute struct. This would allow users to easily interpret declaration attrs of a field as if it were field attrs, etc., without having to jump through hoops and making multiple calls. It also keeps the API tight.

In order to keep the API returning stable when we later introduce more attribute types, this method could take a mask specifying the kind of desired attrs:

type AttrKind struct
const (
    FieldAttr AttrKind = 1 << iota
    DeclAttr
    // Possible future attr kinds
    // ElemAttr
    // FileAttr
    // ValueAttr = FieldAttr|DeclAttr|ElemAttr
)

The Attribute type could then have a Kind method returning this type and Value.Attributes(mask AttrKind) would take a mask to only return the requested attributes.

This is not too far off from the implementation in this CL. The heavy lifting of extracting the attrs has been implemented in internal/core/export, so this should be fairly straightforward.

The Attribute type should have some lower-level non-opinionated functionality, like Body() or Contents() or something, in addition to the Name() method you added (should it be Name or Key? Either way I guess).

Does this sound reasonable?

mpvl · 2021-02-18T15:41:35Z

cue/types.go

+}
+
+// Map returns all key/value pairs for the attribute as a map
+func (a *Attribute) Map() map[string]string {


This method should be removed.

mpvl · 2021-02-18T15:42:29Z

cue/types.go

@@ -2098,11 +2098,45 @@ func (v Value) Attribute(key string) Attribute {
 	return Attribute{internal.NewNonExisting(key)}
 }

+// Attributes reports all field attributes for the Value.
+func (v Value) Attributes() []Attribute {


See CL-level comment.

@verdverm

First attempt by @verdverm, significant edits by @mpvl. The API has been designed so that new attribute types can be added without breaking anything: the user will always have to explicitly specify the types of attributes that are needed. This does not handle duplicate attributes. In the future, there could be merge functionality for this purpose. This could be done by adding option flags if we can settle on what constitutes good options. Original comments from Tony: This enables a user of the Go API to get all attributes so they can inspect them, rather than only being able to ask if a particular attribute exists on a value. Really useful for tools leveraging CUE! I recall @mpvl might have wanted the return to be a Value or List rather than the slice? Closes #529 #529 GitOrigin-RevId: 6a8951a Change-Id: Ie3cc9983656864fa44852b924a3142c82e336f4a

@verdverm

First attempt by @verdverm, significant edits by @mpvl. The API has been designed so that new attribute types can be added without breaking anything: the user will always have to explicitly specify the types of attributes that are needed. This does not handle duplicate attributes. In the future, there could be merge functionality for this purpose. This could be done by adding option flags if we can settle on what constitutes good options. Original comments from Tony: This enables a user of the Go API to get all attributes so they can inspect them, rather than only being able to ask if a particular attribute exists on a value. Really useful for tools leveraging CUE! I recall @mpvl might have wanted the return to be a Value or List rather than the slice? Closes #529 #529 GitOrigin-RevId: 6a8951a Change-Id: Ie3cc9983656864fa44852b924a3142c82e336f4a

@verdverm

First attempt by @verdverm, significant edits by @mpvl. The API has been designed so that new attribute types can be added without breaking anything: the user will always have to explicitly specify the types of attributes that are needed. This also adds the NumArgs, Arg, and RawArg methods to give access to the attribute fields. This does not handle duplicate attributes. In the future, there could be merge functionality for this purpose. This could be done by adding option flags if we can settle on what constitutes good options. Original comments from Tony: This enables a user of the Go API to get all attributes so they can inspect them, rather than only being able to ask if a particular attribute exists on a value. Really useful for tools leveraging CUE! I recall @mpvl might have wanted the return to be a Value or List rather than the slice? Closes #529 #529 GitOrigin-RevId: 6a8951a Change-Id: Ie3cc9983656864fa44852b924a3142c82e336f4a

@verdverm

First attempt by @verdverm, significant edits by @mpvl. The API has been designed so that new attribute types can be added without breaking anything: the user will always have to explicitly specify the types of attributes that are needed. This also adds the NumArgs, Arg, and RawArg methods to give access to the attribute fields. This does not handle duplicate attributes. In the future, there could be merge functionality for this purpose. This could be done by adding option flags if we can settle on what constitutes good options. Original comments from Tony: This enables a user of the Go API to get all attributes so they can inspect them, rather than only being able to ask if a particular attribute exists on a value. Really useful for tools leveraging CUE! I recall @mpvl might have wanted the return to be a Value or List rather than the slice? Closes #529 #529 GitOrigin-RevId: 6a8951a Change-Id: Ie3cc9983656864fa44852b924a3142c82e336f4a

googlebot added the cla: yes label Sep 17, 2020

verdverm mentioned this pull request Sep 17, 2020

Add "Attributes()" to the Go API #413

Closed

verdverm changed the title ~~Add function for Value.Attributes() as a slice~~ Add function for Value.Attributes() Sep 17, 2020

mpvl suggested changes Sep 30, 2020

View reviewed changes

googlebot added cla: no and removed cla: yes labels Oct 5, 2020

googlebot added cla: yes and removed cla: no labels Oct 5, 2020

verdverm requested a review from mpvl October 19, 2020 18:20

verdverm mentioned this pull request Jan 24, 2021

Go API: Get value attributes as a slice #670

Closed

mpvl reviewed Feb 18, 2021

View reviewed changes

verdverm mentioned this pull request Feb 19, 2021

For discussion: Alternative to #dagger:compute syntax dagger/dagger#128

Closed

morlay mentioned this pull request Mar 18, 2021

pkg/native: added utils to make custom pkg easier #841

Closed

verdverm added 5 commits March 22, 2021 13:46

add functions for Attributes as a slice, some helpers

5f8f165

remove SetName twice, improve doc strings for Attribute funcs

8b698c2

rename Attribute.Vals() to Map()

43ce825

refix reverted error

5906ced

fix a doc regression in Attrs

33d442a

mpvl changed the title ~~Add function for Value.Attributes()~~ cue: add function for Value.Attributes() Mar 29, 2021

cueckoo closed this in 792da39 Mar 31, 2021

This was referenced Jul 3, 2021

Placeholder for old PR #529 cue-lang/cue#529

Closed

Go API: Get value attributes as a slice cue-lang/cue#670

Open

Placeholder for old PR #413 cue-lang/cue#413

Closed

Placeholder for old PR #841 cue-lang/cue#841

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cue: add function for Value.Attributes() #529

cue: add function for Value.Attributes() #529

verdverm commented Sep 17, 2020

mpvl Sep 30, 2020

verdverm Oct 5, 2020

mpvl Sep 30, 2020

verdverm Oct 5, 2020 •

edited

Loading

googlebot commented Oct 5, 2020

googlebot commented Oct 5, 2020

verdverm commented Oct 5, 2020 •

edited

Loading

googlebot commented Oct 5, 2020

googlebot commented Oct 5, 2020

verdverm commented Oct 19, 2020

mpvl commented Jan 27, 2021

verdverm commented Jan 27, 2021

mpvl commented Jan 28, 2021

verdverm commented Feb 17, 2021

mpvl commented Feb 18, 2021

mpvl Feb 18, 2021

mpvl Feb 18, 2021

cue: add function for Value.Attributes() #529

cue: add function for Value.Attributes() #529

Conversation

verdverm commented Sep 17, 2020

mpvl Sep 30, 2020

Choose a reason for hiding this comment

verdverm Oct 5, 2020

Choose a reason for hiding this comment

mpvl Sep 30, 2020

Choose a reason for hiding this comment

verdverm Oct 5, 2020 • edited Loading

Choose a reason for hiding this comment

googlebot commented Oct 5, 2020

googlebot commented Oct 5, 2020

verdverm commented Oct 5, 2020 • edited Loading

googlebot commented Oct 5, 2020

googlebot commented Oct 5, 2020

verdverm commented Oct 19, 2020

mpvl commented Jan 27, 2021

verdverm commented Jan 27, 2021

mpvl commented Jan 28, 2021

verdverm commented Feb 17, 2021

mpvl commented Feb 18, 2021

mpvl Feb 18, 2021

Choose a reason for hiding this comment

mpvl Feb 18, 2021

Choose a reason for hiding this comment

verdverm Oct 5, 2020 •

edited

Loading

verdverm commented Oct 5, 2020 •

edited

Loading