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

thema: Initial pass at Go lenses/migrations #187

Merged
merged 8 commits into from
Jul 28, 2023
Merged

Conversation

sdboyer
Copy link
Contributor

@sdboyer sdboyer commented Jul 21, 2023

This introduces an alternate mechanism for writing lenses: passing in a slice of ImperativeLens to BindLineage:

type ImperativeLens struct {
	To, From SyntacticVersion
	Mapper   func(inst *Instance, to Schema) (*Instance, error)
}

where each function has exactly the same scope of responsibility as would otherwise be fulfilled by a declarative lens written in CUE.

The caller provides these via a new BindOption called ImperativeLenses(). In the current implementation, the caller must choose to implement their lenses entirely in Go, or entirely in CUE. We could relax this later.

This isn't fully ready - definitely needs more tests, and the requisite changes to Translate() aren't yet implemented. (Though i think those will be like another 40 LoC...trivial). But the test that exists so far at least verifies the correctness of the checker which ensures all and only the exact set of Go migrations are being provided.

The test also shows what actually writing Go lenses might plausibly look like - though PLEASE NOTE that when this is put to use in kindsys, we'll be putting in a wrapping layer so that the developer doesn't need to interact directly with *thema.Instance.

@sdboyer sdboyer added the enhancement New feature or request label Jul 21, 2023
prior = v
}

// TODO is it worth making each sub-item into its own error type?
Copy link
Contributor

Choose a reason for hiding this comment

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

Personally, I don't see any need (at least for now), to explicitly model every item into its own error, as far as we bring that information back to the consumer.

bind.go Outdated Show resolved Hide resolved
bind.go Outdated Show resolved Hide resolved
surface.go Outdated
// ImperativeLenses takes a slice of [ImperativeLens]. These lenses will be
// executed on calls to [Instance.Translate].
//
// A mix of Go and CUE lenses may be provided, but only one lens may be provided
Copy link
Contributor

@joanlopez joanlopez Jul 21, 2023

Choose a reason for hiding this comment

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

A mix of Go and CUE lenses may be provided

Note: we may need to revisit this text before getting this pull request merged - it doesn't seem to be aligned with what's stated in the pull request description.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, thank you! that's the first iteration leaking through

surface.go Outdated Show resolved Hide resolved
bind.go Outdated Show resolved Hide resolved
bind.go Outdated
// to, from
type lensID [2]SyntacticVersion

func lid(to, from SyntacticVersion) lensID {
Copy link

Choose a reason for hiding this comment

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

similarly, reason for not defining this as from, to? This seems backward, I would say "I am going from a to b", not "I am going to b from a".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not a good one, no. it's all a weird artifact of probably mistakenly trying to be consistent with the expected sort ordering in lenses, where we sort first by to and then by from. (That declared sort ordering requirement only applies for CUE lenses, not for these Go things)

Copy link
Contributor

Choose a reason for hiding this comment

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

Just for curiosity; Could you give more context about the reasons behind such requirement, please? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Requiring the inputs to be manually sorted by the user was the main way we got away from the need to dynamically sort them in CUE - which was the main source of our performance issues fixed in #173

Copy link
Contributor

@joanlopez joanlopez Jul 23, 2023

Choose a reason for hiding this comment

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

Sorry, I meant specifically about the order: first by to and then by from

PS: I guess it's because the heuristics to use that sorted list of lenses is easier this way (first by to), but was asking just for curiosity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ahhh - i'm not attached to that as an ordering, either. My motivation was just that, assuming an ascending version sort order (same as with schemas), it seemed preferable to have forward lenses precede reverse ones in the list.

@sdboyer sdboyer marked this pull request as ready for review July 23, 2023 23:31
@sdboyer
Copy link
Contributor Author

sdboyer commented Jul 28, 2023

i'm gonna merge these, with this note...

Recent discussions have absolutely led me to change my mind about the necessity of having Go lenses as an essential part of Thema. They just make all of what Thema is doing considerably easier to at least get started with, because it allows writing the more complex part (lenses) in a familiar language. All the checkable invariants in the world don't matter if the learning curve is too steep.

I'm not sure that what's in this PR is the final form that imperative lenses should take. It does seem to me like there's a plausible path where we actually handle lacuna resolvers as injected funcs, and that the implementation here could be wholly replaced by that. But this PR does the absolute minimal thing necessary to enable lenses in Go, which gets us started. And because it requires a clean split - either there are imperative lenses or declarative ones, no mixing and matching - we have the simplest foundation for changing it later and minimizing impact on users.

@sdboyer sdboyer merged commit 64e9d78 into main Jul 28, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants