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

Add support for 0.27.0-RC1 and drop pprint for Dotty #383

Merged
merged 3 commits into from
Sep 9, 2020

Conversation

tgodzik
Copy link
Contributor

@tgodzik tgodzik commented Sep 4, 2020

This PR does the following:

  • adds support for 0.27.0-RC1 and switches from 0.26.0-RC1 to 0.26.0
  • removes pprint and sourcecode dependency for Dotty
  • changes to toString for Dotty evaluation
  • uses the default Dotty printer for types, althoguh it need to be copied here, since the reflect printer prints prefixes, which make it unreadable - I plan to add an option in Dotty itself and we will be able to remove that altogether
  • switches to local Text macro for both Dotty and Scala 2.

@tgodzik tgodzik force-pushed the update-dotty branch 4 times, most recently from 0599f0a to f17f2a7 Compare September 4, 2020 15:00
@@ -0,0 +1,15 @@
package mdoc.internal.sourcecode
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 macros are copied from sourcecode.

Copy link
Contributor

@kpbochenek kpbochenek left a comment

Choose a reason for hiding this comment

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

I cloned your fork, changed branch, then I am opening a VSCode, go to WorksheetSuite, run 'test'. All tests fail with:

error: basic.scala:9 (mdoc generated code) Symbol 'type mdoc.interfaces.RangePosition' is missing from the classpath.
This symbol is required by 'class mdoc.document.RangePosition'.
Make sure that type RangePosition is in your classpath and check for conflicting dependencies with `-Ylog-classpath`.
A full rebuild may help if 'RangePosition.class' was compiled against an incompatible version of mdoc.interfaces.
val x = 1.to(4).toVector; $doc.binder(x, 1, 4, 1, 5)

bloop test worksheets <- same problem

@@ -147,25 +177,26 @@ class WorksheetSuite extends BaseSuite {

// From 2.13 we get `name =` part
Copy link
Contributor

Choose a reason for hiding this comment

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

We can't get named params in case classes for dotty? Maybe comment need to change to 'For 2.13 only'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From discussions it turns out it causes a bit too verbose output and since we are not using pprint here, but toString named parameters are out. I will change the comment.

Copy link
Contributor

@kpbochenek kpbochenek left a comment

Choose a reason for hiding this comment

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

Added some minor comments but for a more meaningful review I think you need someone that also looked at adding dotty support because those changes are strongly based on that.

@tgodzik
Copy link
Contributor Author

tgodzik commented Sep 8, 2020

I cloned your fork, changed branch, then I am opening a VSCode, go to WorksheetSuite, run 'test'. All tests fail with:

error: basic.scala:9 (mdoc generated code) Symbol 'type mdoc.interfaces.RangePosition' is missing from the classpath.
This symbol is required by 'class mdoc.document.RangePosition'.
Make sure that type RangePosition is in your classpath and check for conflicting dependencies with `-Ylog-classpath`.
A full rebuild may help if 'RangePosition.class' was compiled against an incompatible version of mdoc.interfaces.
val x = 1.to(4).toVector; $doc.binder(x, 1, 4, 1, 5)

bloop test worksheets <- same problem

I can't really reproduce the issue. Best to change the version to a stable one before publishing.

@kpbochenek
Copy link
Contributor

Maybe I am doing something wrong, but what ? ;/

> bloop about
bloop v1.4.3-23-550c6c0a
Using Scala v2.12.8 and Zinc v1.3.0-M4+45-d4354be3
Running on Java JDK v1.8.0_265 (/usr/lib/jvm/java-8-openjdk/jre)

cat project/metals.sbt
// addSbtPlugin("ch.epfl.scala" % "sbt-bloop" % "1.4.3-23-550c6c0a")

> rm -rf .bloop
> sbt bloopInstall
> bloop test worksheets
tests.worksheets.WorksheetSuite:
error: basic.scala:9 (mdoc generated code) Symbol 'type mdoc.interfaces.RangePosition' is missing from the classpath.
This symbol is required by 'class mdoc.document.RangePosition'.
Make sure that type RangePosition is in your classpath and check for conflicting dependencies with `-Ylog-classpath`.
A full rebuild may help if 'RangePosition.class' was compiled against an incompatible version of mdoc.interfaces.
val x = 1.to(4).toVector; $doc.binder(x, 1, 4, 1, 5)
                                     ^

==> X tests.worksheets.WorksheetSuite.basic  2.831s munit.FailException: /home/kpbochenek/vl/github/others/tgodzik-mdoc/tests/worksheets/src/test/scala/tests/worksheets/WorksheetSuite.scala:391 Obtained empty output!
sbt:mdocRoot> worksheets/test
[info] Compiling 1 Scala source to /home/kpbochenek/vl/github/others/tgodzik-mdoc/tests/worksheets/target/scala-2.12/test-classes ...
tests.worksheets.WorksheetSuite:
  + basic 3.12s
  + lazy 0.565s
  + binders 0.644s
...

Works perfectly fine with sbt Oo

@kpbochenek
Copy link
Contributor

I cloned your fork, changed branch, then I am opening a VSCode, go to WorksheetSuite, run 'test'. All tests fail with:

error: basic.scala:9 (mdoc generated code) Symbol 'type mdoc.interfaces.RangePosition' is missing from the classpath.
This symbol is required by 'class mdoc.document.RangePosition'.
Make sure that type RangePosition is in your classpath and check for conflicting dependencies with `-Ylog-classpath`.
A full rebuild may help if 'RangePosition.class' was compiled against an incompatible version of mdoc.interfaces.
val x = 1.to(4).toVector; $doc.binder(x, 1, 4, 1, 5)

bloop test worksheets <- same problem

I can't really reproduce the issue. Best to change the version to a stable one before publishing.

Wait what? version of what? publishing of what? what has unstable version? ❓

@tgodzik
Copy link
Contributor Author

tgodzik commented Sep 8, 2020

Wait what? version of what? publishing of what? what has unstable version?

So the problem is that we need to publish for 0.27.0 and 2.12 some of the dependecies. Each time the scala version changes the dyn plugin generates a different version, so I am testing it under version:= 2.2.7-SNAPSHOT etc

Copy link
Contributor

@kpbochenek kpbochenek left a comment

Choose a reason for hiding this comment

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

ok assuming it was checked in metals that dotty worksheets still work LGTM!

One thing we can improve now or later would be for newcomer wanting to contribute to explain to them what to do when they clone this repo, add test and cannot run it. (I would just update README.md with something like 'if you try to run tests and see this error "Symbol 'type mdoc.interfaces.RangePosition' is missing from the classpath" then execute this and it should work: '?????')

@tgodzik tgodzik force-pushed the update-dotty branch 2 times, most recently from 4b51a63 to c58fe92 Compare September 9, 2020 11:28
@tgodzik
Copy link
Contributor Author

tgodzik commented Sep 9, 2020

I will go ahead with the merge an release. @olafurpg If something at all breaks I will make sure it's fixed as quickly as possible, but hopefully there shouldn't be any problems since the changes to to the Scala 2 functionalities are really small.

@tgodzik tgodzik merged commit f103ee4 into scalameta:master Sep 9, 2020
@tgodzik tgodzik deleted the update-dotty branch September 9, 2020 12:26
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.

2 participants