Skip to content

Enable scala 2.13 (fixing #344)#368

Merged
cquiroz merged 10 commits intotypelevel:masterfrom
ybasket:enable-scala-2.13
Dec 9, 2019
Merged

Enable scala 2.13 (fixing #344)#368
cquiroz merged 10 commits intotypelevel:masterfrom
ybasket:enable-scala-2.13

Conversation

@ybasket
Copy link
Contributor

@ybasket ybasket commented Nov 24, 2019

WIP as it is based on the not yet merged #363 (will rebase asap, for now just ignore the first two commits) and tests didn't compile for native (seems expected), so I let Travis check.

I found a work-around for scala/bug#11734 (avoiding the indirection via the type aliases in the squants package object, see 8cab981). Based on that the PR contains the usual small things to enable the cross build for 2.13, like adapting the build definitions and introducing a small version dependent trait for the tests as Ordering.FloatOrdering got split in 2.13.

fixes #344

@ybasket ybasket changed the title WIP: Enable scala 2.13 (fixing #344) Enable scala 2.13 (fixing #344) Nov 29, 2019
@ybasket
Copy link
Contributor Author

ybasket commented Dec 2, 2019

@cquiroz could have a look on this one please?

@Shastick
Copy link

Shastick commented Dec 9, 2019

Seconding @ybasket: squants is the last thing holding us back from migrating to 2.13 for several projects :)

Copy link
Collaborator

@cquiroz cquiroz left a comment

Choose a reason for hiding this comment

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

This looks good to me with a minor comment about the imports

import squants._
import squants.space.{ Feet, Millimeters, UsMiles }
import squants.time.{ Seconds, TimeDerivative, TimeIntegral, SecondTimeDerivative, TimeSquared, Time }
import squants.{AbstractQuantityNumeric, Dimension, PrimaryUnit, Quantity, SiUnit, UnitConverter, UnitOfMeasure}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why these changes? Is it due to the IDE reorganizing it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, avoiding the import of squants._ as this was causing the compilation problems due the type aliases defined there. Not beautiful, but has to be like this at least until scala/bug#11734 is resolved.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, thanks for the clarification


object KineticEnergy {
def apply(mass: Mass, velocity: Velocity): Energy =
def apply(mass: Mass, velocity: squants.motion.Velocity): Energy =
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we use the full name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're in the package squants, so the only way to avoid picking up the problematic type aliases in the squants companion object are fully qualified names.

}

object SquantsNumeric {
object SquantsNumeric extends ScalaVersionSpecificNumericInstances {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Great!

@cquiroz
Copy link
Collaborator

cquiroz commented Dec 9, 2019

Thanks @ybasket for your PR
I like to wait for approval of another committer before proceeding, perhaps @garyKeorkunian
Given how important this is, I'll wait just a couple of days before merging and publishing

@garyKeorkunian
Copy link
Contributor

LGTM, good work on finding a way around the issue, @ybasket

@garyKeorkunian
Copy link
Contributor

If there are no objections, I am OK with merging this.

@cquiroz cquiroz merged commit f261396 into typelevel:master Dec 9, 2019
@gvolpe
Copy link

gvolpe commented Dec 10, 2019

Awesome work @ybasket !

@cquiroz what are the plans for a new release? Is there a published snapshot that I can try out?

@cquiroz
Copy link
Collaborator

cquiroz commented Dec 10, 2019

I plan to merge some PRs and do a release today

@gvolpe
Copy link

gvolpe commented Dec 10, 2019

Great, thanks! 🎉

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.

scala 2.13

5 participants