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

provide implicit to query case class companion objects directly #73

Merged
merged 1 commit into from
Dec 28, 2015

Conversation

fwbrasil
Copy link
Collaborator

@mariusmuja @rfranco, I'd like to get your opinion on this if you have some time.

This change allows the user to query case class companions directly through an implicit conversion:

case class Person(id: Int, name: String)
val q = quote {
  Person.filter(_.name == "John")
}

Produces the same query as:

val q = quote {
  query[Person].filter(_.name == "John")
}
  • I'm not sure this should be exposed as part of io.getquill._, I would like to avoid exposing so many implicit conversions.
  • This could make harder to understand quill as it introduces two ways of doing the same thing.

@fwbrasil
Copy link
Collaborator Author

Related issue: #24

@codecov-io
Copy link

Current coverage is 98.69%

Merging #73 into master will increase coverage by +0.07% as of 90d14e4

Powered by Codecov. Updated on successful CI builds.

@mariusmuja
Copy link

I also don't think this should be exposed through io.getquill._ for the reasons you listed above, having to add an explicit import for this feature (import io.getquill.ImplicitQuery._) is preferable.

@godenji
Copy link
Contributor

godenji commented Dec 27, 2015

I would like to avoid exposing so many implicit conversions.

Ha, ha, keep in mind that Slick, the library Quill is competing with for mindshare, has a "few" more implicit conversions than 22 last I checked ;-)

Let's ask a question: in the end what is query[T]? Specifically, will one ever query on a non-product type? (Quill examples say no). If not then query[T] is a syntactic tax on the user.

Just for fun, strip out the query[T]s from Quill examples in the readme...and you have beauty. Now, in real world projects queries will be far more complex than the readme simple case; here's an example from a fork of ScalaQuery (the predecessor to Slick):

def statsBase(season: Column[Year]) = for{
  (p,r)  <- Player join Roster $ (_.playerFk) if r.season =~ season
  (t,_)  <- Team join r $ (_.teamFk)
  (ts,_) <- TeamSeason join t on((ts,_)=> 
    ts.teamId =~ t.id & ts.season =~ r.season
  )
} yield(p,r,t,ts)

Add in query[T] to the above snippet and you have pointless noise cluttering intent. Not sure how it would look in Quill, probably something like:

def statsBase = quote{
  (season: Year) => for{
    (p,r)  <- query[Player] join query[Roster] on(_.id == _.playerId) if r.season =~ season
    (t,_)  <- query[Team] join query[Roster] on(_.id == _.teamId)
    (ts,_) <- query[TeamSeason] join query[Team] on((ts,_)=> 
      ts.teamId =~ t.id & ts.season =~ r.season
    )
  } yield(p,r,t,ts)
}

and that's the relatively simple case! Queries involving 10+ tables, those query[T]s really start to get in the way.

Given the early stage of the project I'd say make implicit conversions the default now and not require users to explicitly import the conversion. Much cleaner solution all around, IMO.

@fwbrasil
Copy link
Collaborator Author

Ha, ha, keep in mind that Slick, the library Quill is competing with for mindshare, has a "few" more implicit conversions than 22 last I checked ;-)

I can't say that I consider Slick a good example in API design. :)

Let's ask a question: in the end what is query[T]? Specifically, will one ever query on a non-product type? (Quill examples say no). If not then query[T] is a syntactic tax on the user.

It also provides table and column name mapping:

val people = quote(query[Person]("people", _.name -> "person_name"))

Just for fun, strip out the query[T]s from Quill examples in the readme...and you have beauty. Now, in real world projects queries will be far more complex than the readme simple case; here's an example from a fork of ScalaQuery (the predecessor to Slick):

def statsBase(season: Column[Year]) = for{
(p,r) <- Player join Roster $ (.playerFk) if r.season =~ season
(t,
) <- Team join r $ (.teamFk)
(ts,
) <- TeamSeason join t on((ts,_)=>
ts.teamId =~ t.id & ts.season =~ r.season
)
} yield(p,r,t,ts)

This doesn't seem better than what Quill already provides. For instance, if I'm not mistaken, Player needs to be created somewhere as val Player = TableQuery[PlayerRow]. The same way, you could create a quotation val Player = quote(query[PlayerRow]) and reuse it across all queries. Am I missing something?

@mariusmuja
Copy link

Just for fun, strip out the query[T]s from Quill examples in the readme...and you have beauty.

You can get that right now by defining a schema object as described in the docs. And with this merged in you would be able to get that with just one extra import statement.

On the other hand, if this conversion was enabled by the default, imagine trying to use quill with some other library that decided having default implicit conversions from case classes was a good idea.

@rfranco
Copy link
Contributor

rfranco commented Dec 27, 2015

I have a concern about to there are differents ways to do the same thing, usually that give alot of doubts to who will use it.
Another point is that lose the possible to change the table name and column name.

@fwbrasil
Copy link
Collaborator Author

I've decided to keep it as a separate import (io.getquill.ImplicitQuery._), considering the overall feedback.

fwbrasil added a commit that referenced this pull request Dec 28, 2015
provide implicit to query case class companion objects directly
@fwbrasil fwbrasil merged commit 017a21c into master Dec 28, 2015
@fwbrasil fwbrasil deleted the implicit-query branch December 28, 2015 04:25
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