-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Preliminary Munit port #3538
Preliminary Munit port #3538
Conversation
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.
Took a quick look. Search/replace seems to have gone a bit awry. Also it would be nice if we didn't have the unnecessary parentheses around the ===
operand.
Codecov Report
@@ Coverage Diff @@
## master #3538 +/- ##
==========================================
- Coverage 91.34% 91.32% -0.02%
==========================================
Files 386 386
Lines 8592 8592
Branches 241 269 +28
==========================================
- Hits 7848 7847 -1
- Misses 744 745 +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.
I only took a very cursory look
@@ -126,6 +127,7 @@ lazy val commonJsSettings = Seq( | |||
jsEnv := new org.scalajs.jsenv.nodejs.NodeJSEnv(), | |||
// batch mode decreases the amount of memory needed to compile Scala.js code | |||
scalaJSLinkerConfig := scalaJSLinkerConfig.value.withBatchMode(isTravisBuild.value), | |||
scalaJSLinkerConfig ~= (_.withModuleKind(ModuleKind.CommonJSModule)), |
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.
Why is this change necessary?
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 honestly don't know, but the error message I got when running the tests without it pointed me to this. 😄
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 leave just few minor comments. Practically just style.
Co-authored-by: Filippo Mariotti <barambani@users.noreply.github.com>
Co-authored-by: Filippo Mariotti <barambani@users.noreply.github.com>
Co-authored-by: Filippo Mariotti <barambani@users.noreply.github.com>
Co-authored-by: Filippo Mariotti <barambani@users.noreply.github.com>
Co-authored-by: Filippo Mariotti <barambani@users.noreply.github.com>
Co-authored-by: Filippo Mariotti <barambani@users.noreply.github.com>
Co-authored-by: Filippo Mariotti <barambani@users.noreply.github.com>
Co-authored-by: Filippo Mariotti <barambani@users.noreply.github.com>
Co-authored-by: Filippo Mariotti <barambani@users.noreply.github.com>
Thanks for those, I think unblocking the dotty work is more important than fixing all the style issues for now, but I'm sure some regex master could fix this for us at some point 😄 |
For sure. Let's ship this 🚀 |
Alright, merging this :) |
Should resolve #3534