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

Reassign transformer methods to FilterBuilder #25

Merged
merged 2 commits into from Jan 31, 2022

Conversation

boatilus
Copy link
Contributor

In lieu of a more ergonomic option within Go's type system (that feels reasonable) to resolve #8, I'm proposing removing TransformBuilder and simply reassigning its associated methods to FilterBuilder. As you'd expect, this lets you do:

c.From("users").Select("*", "", false).Limit(10)

Of course, it also somewhat unfortunately lets you do:

c.From("users").Select("*", "", false).Limit(10).Eq("name", "sean")

Which obviously looks weird.

Since the transformer methods aren't usable in the current release, this is probably also a good time to discuss some of ergonomics around the transformer methods. Having four params with type similarities in Order, for example, feels a bit unnatural, so I'd propose having it instead accept a struct of options (or nil to get the defaults).

type OrderOpts struct {
	Ascending    bool
	NullsFirst   bool
	ForeignTable string
}

func (f *FilterBuilder) Order(column string, opts *OrderOpts) *FilterBuilder

This also roughly mirrors the interface in postgres-js. I'm unsure whether it'd be useful to also update Limit and Range similarly, given they both also currently accept foreignTable.

A full PR would need tests/docs -- this is just aimed at opening up discussion.

@yusufpapurcu
Copy link
Member

This solution LGTM. Also requiring struct is a nice solution. But I think we don't need same thing in Limit and Range.

@boatilus boatilus marked this pull request as ready for review January 28, 2022 20:37
@yusufpapurcu
Copy link
Member

LGTM!
Thanks for support @boatilus 💯

@yusufpapurcu yusufpapurcu merged commit 408fb4e into supabase-community:main Jan 31, 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.

Implement a Single method
2 participants