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

Allow repo opts to be passed to paginate/2 #102

Closed
wants to merge 1 commit into from

Conversation

dgigafox
Copy link

Right now we only can pass :prefix repo opt to Repo.paginate/2 but it should behave like a normal repo operation where the user can pass arbitrary/predefined repo options such as those indicated in https://hexdocs.pm/ecto/Ecto.Repo.html#module-shared-options

For example, we should be able to pass :log and :admin as opts to Repo.paginate/2 like this:

Scrivener.Ecto.Repo.paginate(Post, options: [log: true, admin: true])

Now the query will be logged and we can use the arbitrary opts such as admin in the prepare_query/3 callback like so:

@impl true
def prepare_query(_operation, query, opts) do
  if opts[:admin] do
    {query, opts}
  else
    query = from(x in query, where: is_nil(x.deleted_at))
    {query, opts}
  end
end

Repo.paginate/2 should behave like a normal repo operation where user
can pass arbitrary/predefined repo options such as those indicated in https://hexdocs.pm/ecto/Ecto.Repo.html#module-shared-options
@bruno-azenha
Copy link

This would be a lovely feature to have.
My use case is trying to pass the timeout option.

@DougVonMoser
Copy link

Neat! I am also hoping for the timeout option to be passable.

@dgigafox are you able to manually run tests on your PR screen? or maybe push an empty commit to trigger the test workflow please?

The maintainer added a test hook for PRs in June after this was opened in May. I'm curious if green tests would make this easier to merge 🙏

Thanks!

@DougVonMoser
Copy link

Oh wait, your commit was a year later 🤦 . nevermind me 😅

Parker-Bartlett added a commit to blockit/scrivener_ecto that referenced this pull request Jan 22, 2024
Parker-Bartlett added a commit to blockit/scrivener_ecto that referenced this pull request Jan 22, 2024
@yordis
Copy link
Contributor

yordis commented Oct 10, 2024

@cpjolicoeur is there an opportunity to get this to merge and released? Do you need help here?

We need this to pass the telemetry_options

@cpjolicoeur
Copy link
Member

@yordis @dgigafox Yes, if we get the conflicts in this branch cleaned up we can merge it in. Apologies.

@yordis
Copy link
Contributor

yordis commented Oct 10, 2024

@cpjolicoeur would you like me to do? I am not sure if @dgigafox is still around after few years

@cpjolicoeur
Copy link
Member

@cpjolicoeur would you like me to do? I am not sure if @dgigafox is still around after few years

Please, if this is something you are interested in carrying forward, you can take it over and/or make a new PR that supersedes/updates this one.

We (MojoTech) recently took over maintenance of the scrivener projects and haven't yet gone through and triaged all the old/existing issues and pull requests yet.

If this is a PR of use to you, please give us a hand, and continue to move it forward and we can get it in and released.

@yordis
Copy link
Contributor

yordis commented Oct 10, 2024

@cpjolicoeur, trying to improve the situation, I am a bit confused; what is that caller configuration that I can not find anywhere in the Ecto docs? Would you be able to guide me here?

@cpjolicoeur cpjolicoeur changed the base branch from master to main October 14, 2024 13:23
cpjolicoeur
cpjolicoeur previously approved these changes Oct 17, 2024
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.

5 participants