-
-
Notifications
You must be signed in to change notification settings - Fork 286
Scala 2 13 #1118
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 2 13 #1118
Changes from all commits
2d00836
3698f4a
d3fa392
727868f
201ab55
19910b0
00e56aa
fb3f2c7
fb60a2a
1c0847f
78f0569
ee2eb94
fe731e2
3c2e830
5b7bebd
83695ea
e006668
49a1541
eb494c7
ba178a5
4278290
bc9ed42
77bdc13
fa241fb
5076ed3
d48728e
1b141a9
0114c4e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -100,7 +100,7 @@ FNH:3 | |
DA:3,1 | ||
DA:5,6 | ||
DA:9,8 | ||
DA:46,4 | ||
DA:41,4 | ||
LH:4 | ||
LF:4 | ||
end_of_record | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -51,11 +51,11 @@ test_transitive_deps() { | |
|
||
test_repl() { | ||
bazel build $(bazel query 'kind(scala_repl, //test/...)') | ||
echo "import scalarules.test._; HelloLib.printMessage(\"foo\")" | bazel-bin/test/HelloLibRepl | grep "foo java" && | ||
echo "import scalarules.test._; TestUtil.foo" | bazel-bin/test/HelloLibTestRepl | grep "bar" && | ||
echo "import scalarules.test._; ScalaLibBinary.main(Array())" | bazel-bin/test/ScalaLibBinaryRepl | grep "A hui hou" && | ||
echo "import scalarules.test._; ResourcesStripScalaBinary.main(Array())" | bazel-bin/test/ResourcesStripScalaBinaryRepl | grep "More Hello" | ||
echo "import scalarules.test._; A.main(Array())" | bazel-bin/test/ReplWithSources | grep "4 8 15" | ||
echo "import scalarules.test._; HelloLib.printMessage(\"foo\")" | bazel-bin/test/HelloLibRepl -Xnojline | grep "foo java" && | ||
echo "import scalarules.test._; TestUtil.foo" | bazel-bin/test/HelloLibTestRepl -Xnojline | grep "bar" && | ||
echo "import scalarules.test._; ScalaLibBinary.main(Array())" | bazel-bin/test/ScalaLibBinaryRepl -Xnojline | grep "A hui hou" && | ||
echo "import scalarules.test._; ResourcesStripScalaBinary.main(Array())" | bazel-bin/test/ResourcesStripScalaBinaryRepl -Xnojline | grep "More Hello" | ||
echo "import scalarules.test._; A.main(Array())" | bazel-bin/test/ReplWithSources -Xnojline | grep "4 8 15" | ||
Comment on lines
+54
to
+58
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the TLDR of these changes? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, should we be passing this arg in the production wrapper of the REPL? Not 100% sure but came to think about it when I wanted to ask you to document this but then thought- why document when we can fix. WDYT? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe it's a choice of user if they want to have jline on the classpath and use it. Probably if someone cares about repl functionallity, this should be properly solved by adding dep provider for repl first. |
||
} | ||
|
||
test_benchmark_jmh() { | ||
|
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 remove 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.
It's a misleading leftover from repositories refactoring. Also the last supproted
org_psywerx_hairyfotr__linter
is for 2.12.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.
So maybe I didn't notice that before (in the repositories refactoring) but from the comment it still sounds like we need to test that we don't include source jars to the classpath
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.
@liucijus not for this PR can you please check this? We either need to remove
# "org_typelevel__cats_core"
and move the comment up or we should uncomment this and remove thejvm_maven_import_extrernal
from aboveThere 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.
Probably it's ok to stay without this comment as it is the duplicate of 1b141a9
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.
On master, line 183
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.
@ittaiz do you expect me to do anything more regarding this thread?
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.
Yes, this commnet still stands. I can merge it and we'll do a followup PR just for it (a bit of a shame but I can live with it). What do you prefer?
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 prefer merge
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.
Merging now. Please followup on this