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

Filtering: better "read" fns #120

Open
Wulf opened this issue Nov 10, 2023 · 7 comments
Open

Filtering: better "read" fns #120

Wulf opened this issue Nov 10, 2023 · 7 comments

Comments

@Wulf
Copy link
Owner

Wulf commented Nov 10, 2023

I just wanted to share my thoughts on the 'read/paginate' functions:

They are very arbitrary unless we introduce a way to filter the results.

In #103, I attempted to add filter() helper which allows us to generate arbitrary WHERE clauses; however, it currently only supports "equals"-based clauses, and not things like:

  • string search
  • date/time start/end/range
  • between/greater-than/less-than number,
  • etc; essentially the full list of SQL operations allowed in WHERE clauses.

This is a major turning point for dsync which has primarily been a code-generation tool as opposed to a query-builder. Deciding to add filter() is the only way functions like read() and paginate() make sense. However, we can only go so far with the filtering before we end up with a query builder 😅.

So here's the thing: do we continue with this effort?

Some more thoughts: we should consider generating cursor-based pagination functions and may also want to split read into: read_first and read_all (or similar-named functions).

Anyway, no big deal, hope all is well 🙌

@Wulf
Copy link
Owner Author

Wulf commented Nov 10, 2023

Using the read() function to fetch a record with a specific primary key is a good-enough use case, so perhaps another option is to remove the paginate() fn?

@hasezoey
Copy link
Collaborator

Using the read() function to fetch a record with a specific primary key is a good-enough use case, so perhaps another option is to remove the paginate() fn?

i would likely be in favor of removing .paginate (and related things in #103), unless we make it explicit that the default paginate function is meant to paginate without a specific clause (table order).
as for the other functions, i think it is reasonable to have some CRUD functions based on the identifier(s)

@Wulf
Copy link
Owner Author

Wulf commented Nov 11, 2023

Perhaps we can put them behind a feature flag and keep them in experimental/unstable state

@hasezoey
Copy link
Collaborator

Perhaps we can put them behind a feature flag and keep them in experimental/unstable state

that would also be a option, should it be a default though? and if disabled, should the old code be generated?

@Wulf
Copy link
Owner Author

Wulf commented Nov 12, 2023

Yeah, let’s leave it disabled by default.

That is to say: in the current version, this would only generate the paginate fn if some experimental flag was set. If we want to stabilize parts of these experiments in the future, we’ll put them behind specific flags or add them to the default set, according to our decision

@hasezoey
Copy link
Collaborator

That is to say: in the current version, this would only generate the paginate fn if some experimental flag was set. If we want to stabilize parts of these experiments in the future, we’ll put them behind specific flags or add them to the default set, according to our decision

so if i understand correctly, we will remove the .paginate function from non-flag builds?

@Wulf
Copy link
Owner Author

Wulf commented Jan 15, 2024

See #126 -- removed .paginate from default set and also introduced some experimental API behind advanced-queries

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

No branches or pull requests

2 participants