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 Scala conversions as both Trait & Object #5965

Closed
wants to merge 2 commits into from

Conversation

tekacs
Copy link

@tekacs tekacs commented Mar 12, 2017

With Conversions available as a trait, downstream users can mix the trait into their own extension classes, to minimise imports.

Due to the object Conversions extends Conversions, functionality is unchanged for users who don't want to make use of this.

Usage example:

package somepackage

import org.jooq.Record
import org.jooq.scalaextensions.Conversions

object MyFancyJooqExtensions extends Conversions {
  implicit class ExtendRecordWithSomething(record: Record) {
    ...
  }
}

// in another file...

import somepackage.MyFancyJooqExtensions._ // Conversions are also provided by this import.

Edit: I had to turn the implicit classes into reference (from value) classes - see my note in my second commit about the (should be limited) effect of that.

With `Conversions` available as a trait, downstream users can mix the trait into their own extension classes, to minimise imports.

Due to the `object Conversions extends Conversions`, functionality is unchanged for users who don't want to make use of this.

Usage example:

```scala
package somepackage

import org.jooq.Record
import org.jooq.scalaextensions.Conversions

object MyFancyJooqExtensions extends Conversions {
  implicit class ExtendRecordWithSomething(record: Record) {
    ...
  }
}

// in another file...

import somepackage.MyFancyJooqExtensions._ // Conversions are also provided by this import.
```
In order to take on a non-static scope (and so reference their outer class), the implicit classes need to stop being `AnyVal`.

This may cause a very small performance regression (one would hope the allocation regression would be caught by escape analysis).

On top of the underlying jOOQ library (in Java, without value classes), I would expect any difference to be negligible, though it can be tested.
@lukaseder
Copy link
Member

Thank you very much for your suggestion. I remember this practice from the good old Java 1.3 days when I started working with Java:

With Conversions available as a trait, downstream users can mix the trait into their own extension classes, to minimise imports.

Ultimately, this practice has been abandoned once IDE support became better and static imports became available.

Is this really something that is done frequently in Scala these days? I'd really avoid introducing a breaking change for what appears to be an almost negligible improvement...

@tekacs
Copy link
Author

tekacs commented Mar 13, 2017

In Scala, we tend to import a lot of implicits. As well as this, implicits can't automatically be 'looked up' for import by your IDE (or even in compiler errors) - static imports and IDE support are non-existent here! As such, being able to put implicits in a well-known place is super valuable. 😄

To put that in perspective, when a downstream user or developer on my team is using the DSL, writing:

db.selectFrom(CROISSANTS).where(CROISSANTS.ID === cross.id).one

This is using two sets of implicits - one to provide === and one for .one on the end. In the event that either of these implicits isn't imported, they just get an error and no assistance. They're left to go hunting amongst the imports of other files, trying imports at random until they hit upon the one that provides the necessary implicit (if they're a little smarter, they'll hit 'Go to Definition' on another usage of '===' in the project and import whatever object they end up in).

Perhaps now you can see how valuable it is therefore, to have the developer import:

import model.jooq.DSL._ // our merge/re-export object

and have all the necessary implicits dropped into scope for them, saving them from copy-pasting random imports to make implicits work.

@lukaseder
Copy link
Member

Thanks for your explanations. It does make sense, but I still don't feel at ease with the suggestion because:

  1. It's not backwards-compatible
  2. As an API maintainer, exposing an interface/trait is much more "costly" than exposing a class in terms of what users who implement it might expect in terms of backwards-compatibility

I'll put this on hold to see if there's enough traction for this kind of change in the community to perhaps do it in the next major release.

@tekacs
Copy link
Author

tekacs commented Mar 18, 2017

Okay - we'll make do with copying the contents out of your object for now and revisit this when you touch it again. :)

@lukaseder
Copy link
Member

Yes, that's perfect. Let's hope we'll get more feedback on this, soon!

@lukaseder lukaseder closed this Jun 15, 2020
@lukaseder
Copy link
Member

The PR got closed by github due to a recent rename of the main branch.

This issue hasn't seen a lot of feedback yet, and in my most recent comments, I was wary of whether the change was reasonable in terms of backwards compatibility.

I will take the opportunity to keep the PR closed as "won't fix"

@lukaseder lukaseder added this to In progress in 3.14 Other improvements via automation Jun 16, 2020
@lukaseder lukaseder moved this from In progress to Done in 3.14 Other improvements Jun 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants