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

Simplify fragments #1881

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

sideeffffect
Copy link
Contributor

@sideeffffect sideeffffect commented Jun 18, 2023

I've rethought the recent changes we've made in recent time to fragments. And I think I have good news: I think we can simplify fragments quite a lot.

  • We don't need to artificially constrain the use only to Reducibles. Most of the combinators work sensibly even for the empty case, i.e. Foldables
  • The problem with the default value on empty case we can solve with a parameter with default value
  • I've used custom Fragment combinators that returned Option[Fragment] in the past, and they have been nothing but pain to work with. It's so so much better design to always return just Fragment from Fragment combinators -- it's lower complexity and easier to work with for users.

# Conflicts:
#	modules/core/src/main/scala/doobie/util/fragments.scala
#	modules/core/src/test/scala/doobie/util/FragmentsSuite.scala
@mergify mergify bot added the core label Jun 18, 2023
}

test("Usage test: whereAndOpt") {
assertEquals(whereAndOpt(Some(fr"hi"), orOpt(List.empty[Fragment]), orOpt(List(fr"a", fr"b"))).query[Unit].sql, "WHERE hi AND (a OR b ) ")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you think given this example is painful to work with? I feel like this is alright?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or course, this small example is fine as such. But in more complex queries, the options are more annoying to work with.

With this PR taken into account, you can express the same idea like this:

whereAnd(Some(fr"hi"), orOpt(List.empty[Option[Fragment]], onEmpty = true), or(List(fr"a", fr"b")))

@jatcwang
Copy link
Collaborator

jatcwang commented Jun 19, 2023

  • I thought about adding a isEmpty to Fragment so we can use it as a way to filter out "empty" fragments, but ultimately rejected it because I still want "emptiness" to be reflected in the type
  • The main issue I had with or/and taking potentialy-empty collections is that it doesn't prompt the user to think about the empty case. Using a default parameter makes it slight more visible, but still doesn't solve that.

@guymers
Copy link
Contributor

guymers commented Jun 19, 2023

Can we just revert back to what was in 1.0.0-RC2? It was simple, one method for each and no boolean parameters with default values...

@sideeffffect
Copy link
Contributor Author

One specific example that I've just come across where returning Option[Fragment] makes live unnecessarily difficult:

sql"""
  ...
  ${Fragments.andOpt(emailIsNull, substring)}
  ...
"""

You can't interpolate this fragment. It fails, because the result is of andOpt is Option[Fragment]. It would work only if it were a mere Fragment.

@guymers
Copy link
Contributor

guymers commented Jul 31, 2023

${Fragments.andOpt(emailIsNull, substring).getOrElse(sql"TRUE")}
or
${Fragments.andOpt(emailIsNull, substring).orEmpty}
depending on the context?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants