APP-15248 Update transforms to accept arbitrary IDs#399
APP-15248 Update transforms to accept arbitrary IDs#399
Conversation
🦋 Changeset detectedLatest commit: cae5779 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
draw/transform.go
Outdated
| idBytes = parsedId[:] | ||
| } else { | ||
| // If the id is not a UUID, use it to generate a new UUID | ||
| newId := uuid.NewSHA1(transformNamespace, []byte(id)) |
There was a problem hiding this comment.
open for discussion, but I am not a huge fan of silently fixing the uuid, I think that will probably lead to people providing bad ids, this succeeding but then not being able to query for the transform since they thought the bad Id they gave was the one used
I think we should either return the error to the user if they give a bad uuid or better just don't take in the id param at all and generate one in the draw api that the user can extract from the returned transform if they want
There was a problem hiding this comment.
Yeah, I thought about that. My reasoning here is more of a UX/ergonomics one, and not one I am married to, or maybe we should move up to the client level.
My thinking, as someone who was using these APIs to write the client code and tests, is that it adds a lot of boilerplate and annoyance to import a UUID library, create a UUID, then convert it to a string every time I want to draw something with updates/persistence. It was equally as annoying to accept an empty byte array when a standard uuid wasn't used. I might also have some other case where I want to assign some level of persistent ID but can't easily pass them down through, say, drawing geometries in a frame, where I may want to update those geometries after manipulating the frame instead of redrawing them.
The other option I was thinking of was to use the same optional-parameter pattern we have: the default is that id is an empty string, then have WithUUID, which expects an actual UUID, and WithID, which accepts an arbitrary ID string.
There was a problem hiding this comment.
Thinking about it, I like that better since we are passing empty strings in a lot of places and that is annoying DX. Check out the updates.
…48-update-transforms-to-use-arbitrary-ids
…48-update-transforms-to-use-arbitrary-ids
2 participants
NewTransformto accept an arbitrary ID value that it will use to generate a UUID in the transform namespaceTesting
I haven't really exposed this in any external API yet so should just be checking things work as expected.