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

Cross compiles project to Scala 3 #227

Merged
merged 2 commits into from
Feb 22, 2021

Conversation

amumurst
Copy link
Contributor

@amumurst amumurst commented Aug 22, 2020

This PR contains a full reimplementation of all macros for Dotty.

This is the first time I write any macros at all for scala, so its highly likely that they are somewhere on the scale from completely wrong to somewhat okay 😄

Else there are some slight changes to the test classes to get Dotty running.

The "API" of the loggers is the same, but I have absolutely no clue if things are breaking or not 😅

@lightbend-cla-validator
Copy link
Collaborator

At least one pull request committer is not linked to a user. See https://help.github.com/en/articles/why-are-my-commits-linked-to-the-wrong-user#commits-are-not-linked-to-any-user

@amumurst
Copy link
Contributor Author

Travis breaks unrelated because it can't find openjdk11, attempted fix in #228.

import util._

message.unseal match{
case Inlined(_, _, Apply(Select(Apply(Select(Select(_, "StringContext"), _), messageNode), _), argumentsNode)) =>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seems so wrong with such a deep nesting of destructuring, any hints on how to not do this is appreciated.

trait Tagged[U]
type @@[+T, U] = T with Tagged[U]

class Tagger[U] {
def apply[T](t: T): T @@ U = t.asInstanceOf[T @@ U]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dotty did not like this (cannot cast Tagged[U] to String, or something allong those lines.)


def errorMessageArgs(underlying: Expr[Underlying], message: Expr[String], args: Expr[Seq[Any]]) (using QuoteContext) = {
val anyRefArgs = formatArgs(args)
if(anyRefArgs.isEmpty)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The isEmpty cases here are not present in scala 2.x macros, but the tests do fail without them (delegates to wrong function).
With out it the error is that it triggers
public void error(String format, Object... arguments); with no arguments
but it should trigger
public void error(String msg);

I do not understand how the 2.x macros do not run in to the same issue 🤷

Copy link
Contributor

@sullis sullis left a comment

Choose a reason for hiding this comment

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

let's upgrade to 3.0.0-M3

project/plugins.sbt Outdated Show resolved Hide resolved
project/Dependencies.scala Outdated Show resolved Hide resolved
.travis.yml Outdated Show resolved Hide resolved
@SethTisue SethTisue changed the title Cross compiles project to dotty Cross compiles project to Scala 3 Feb 3, 2021
@SethTisue
Copy link
Collaborator

SethTisue commented Feb 3, 2021

Hi. I have merge rights in this repo now, and I'll be happy to merge this and pull publishing levers if it is rebased (once #253 is merged) and updated for Scala 3.0.0-M3.

@SethTisue
Copy link
Collaborator

See #225 for discussion about the possible future of this repository.

@SethTisue
Copy link
Collaborator

(needs rebase)

@SethTisue
Copy link
Collaborator

Oh, and Scala 3.0.0-RC1 just hit Maven Central yesterday!

@amumurst
Copy link
Contributor Author

Yeah! It compiles for RC1 but scalatest stuff is not out yet, probably can wait for those. Working on rebase atm.

@SethTisue
Copy link
Collaborator

@amumurst (I think it would be fine to land this for M3, then separately land an RC1 upgrade)

@SethTisue
Copy link
Collaborator

I've requested that the ScalaTest folks publish for RC1: scalatest/scalatest#1138 (comment)

 - Split macro to 2 2.X folder for old macro definitions and 3.X for scala 3 macros
 - Add LoggerFixture to test for structured fixtures
@amumurst
Copy link
Contributor Author

@bbonanno I had to kinda revert back to scalatestplus-mockito from mockito-scala since from what I kan see it is not doing anything about scala 3 so far (mockito/mockito-scala#330.)

Syntax-wise its just replacing imports and * to any, but I understood from #255 that the scala version is safer to use.

@ultrasecreth
Copy link
Contributor

@amumurst sadly I'm blocked by a ton of dependencies to progress to support Scala 3, in the same way you were by mockito-scala, so I totally feel your pain.
Yes, it's safer, but I guess the project can do without it until I can add Scala 3 support.

The PR had also changes to tests that were broken regardless of the mockito flavour, so I guess you wanna keep those changes

@amumurst
Copy link
Contributor Author

@bbonanno Totally!
All changes you did are kept and working, only this https://github.com/lightbend/scala-logging/pull/227/files#diff-33835318a9975954275bb9500a41c274042970ee83606cf05c2e41a8c6bc6616R31 had to be changed in the actual test implementations 👍

Copy link
Collaborator

@SethTisue SethTisue left a comment

Choose a reason for hiding this comment

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

build changes LGTM. (I'm unable to review the code changes.)

@SethTisue
Copy link
Collaborator

I'm happy to merge regardless, if it comes to that, but let's let it sit until next week to see if any reviewers appear.

project/Dependencies.scala Outdated Show resolved Hide resolved
.travis.yml Outdated Show resolved Hide resolved
@SethTisue SethTisue merged commit f1a636f into lightbend-labs:master Feb 22, 2021
@SethTisue
Copy link
Collaborator

👏

@SethTisue SethTisue mentioned this pull request Mar 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants