-
Notifications
You must be signed in to change notification settings - Fork 100
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
Scala 3 #490
Scala 3 #490
Conversation
Is this problem still current? |
No, it is fixed
|
I'm superbusy till next wednesday. If someone want to help, I can add a collaborator to my fork |
41ff4b6
to
7092a2a
Compare
b786953
to
4554e94
Compare
Hi, is someone working on the code review? Can we do something to help/facilitate it? |
I guess nobody is going to review it, so we should leave it as it is and publish a candidate like M1 or RC1 |
@barkhorn @paulbutcher Guys, all this is ridiculous, I think we can manage it without waiting another few years passing by |
Please release milestone artifacts with Scala 3 support. |
I'm guessing the maintainers aren't seeing these notifications. Has anyone tried contacting them by other means? |
I can take a look at the weekend, but don't have proper access to things at the moment. If anyone else wants to start put some comments in for now, that would be appreciated. Given the size of the change, would make sense to have some more eyes on it. |
} | ||
}, | ||
scalacOptions ++= Seq("-deprecation", "-unchecked", "-feature", "-Xcheckinit", "-target:jvm-1.8") | ||
scalaVersion := "3.3.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scalaVersion := "3.3.0", | |
scalaVersion := "3.3.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a bug in 3.3.0 which allows us not to use @experimental
on every test suite. I think we should use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth a comment, then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
I noticed that the |
I'll add a comment, thought I did. |
Thanks for your activity here both, I was about to start writing scalamock out of my project to get to Scala 3 but what you're doing here gives me hope 🙏🏻 |
Co-authored-by: Seth Tisue <[email protected]>
Co-authored-by: Seth Tisue <[email protected]>
assertResult("it worked") { m.byNameParam(42) } | ||
} | ||
} | ||
|
||
/* | ||
|
||
//! TODO - in scala 3 we can't distinguish abstract var setters with non abstract vars setters |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should go into the documentation pages as a known caveat
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added to readme for now
@@ -0,0 +1,189 @@ | |||
// Copyright (c) 2011-2015 ScalaMock Contributors (https://github.com/paulbutcher/ScalaMock/graphs/contributors) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'd remove the year in this line on new files, but not a big deal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
assert(new MatchEpsilon(1.0) == 1.0001) | ||
assert(new MatchEpsilon(1.0) == 1.0001f) | ||
assert(new MatchEpsilon(1.0) == 1) | ||
assert(new MatchEpsilon(1.0).equals(1.0)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is .equals()
different to ==
?
what is this change meant to do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -216,16 +199,16 @@ class MockTest extends AnyFreeSpec with MockFactory with Matchers { | |||
} | |||
} | |||
|
|||
//! TODO - currently doesn't work because we can't override concrete vars |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
worth adding this to the docs, including a brief example, and, if suitable, a workaround?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added migration notes to readme of this repo
"MockFactory" should "be mixed only with Any*Spec and not Async*Spec traits" in { | ||
assertCompiles("new AnyFlatSpec with MockFactory") | ||
assertDoesNotCompile("new AsyncFlatSpec with MockFactory") | ||
//assertDoesNotCompile("new AsyncFlatSpec with MockFactory") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of commenting this out, can we mark the test as skipped please?
or at least add a FIXME as that first assertion still holds true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added FIXME with issue
- name: Set up JDK | ||
uses: actions/setup-java@v3 | ||
with: | ||
java-version: 11 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is JDK 11 still the best runtime?
17/21 are available as LTS versions now but the class-version may be too high for Scala?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, we can user latter version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Play framework still supports JDK 11 and I use ScalaMock in a library released for Play, it would be awesome supporting JDK 11 for a while
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left 11
A lot of work has clearly gone into that, thank for cleaning up in general. It looks good to me. As a general comment, I think it would be great if the documentation could follow to explain any caveats with some of the changes. That doesn't have to be right now but I hope someone would pick that up. |
Should I do anything else so we could proceed? |
something has changed in one of the sbt plugins i think. couldn't release to sonatype just now because of missing signature (why is that even still a requirement...). I'll take a look over the coming days to push it out |
released! 🚢 |
I'm probably just jumping the gun before the repositories have updated but it's:
|
it's on maven central as usual, so you can reference it the same way: https://central.sonatype.com/artifact/org.scalamock/scalamock_3 |
Thanks, I didn't have the Edit: |
great work @goshacodes! really uplifting news this got out 💟 |
Pull Request Checklist
Purpose
Migration to Scala 3.
fix varsvars support is mostly dropped override modifier not allowed for abstract vars scala/scala3#18692var m = mock[T]
+
fix java interfaces and classesworkaround for java classes Can't get constructor of java defined class in reflect api scala/scala3#18694Write additional tests for:
Migration notes:
Abstract vars are not supported anymore, use
scala.compiletime.uninitialized
instead.There are multiple issues when trying to override vars, so
mocking vars
feature is dropped.