Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Bundle scala cli in scala command #20351
Bundle scala cli in scala command #20351
Changes from all commits
4426f99
8fda9cf
f6c27b6
0bf7e75
db7c254
709e91f
b01a91f
84c6969
aeafc08
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
After discussion with @Gedochao, we should not be filtering arguments here
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 is just a test command used only for testing, it automatically adds
--power
,--offline
and--server=false
to commands that support it. this isn't shipped with codeThere 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.
Yep, this is actually an old comment that I forgot to remove. Sorry for that
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 still don't feel this is necessary... passing those arguments explicitly in the tests would make them more readable IMO, rather than embedding them in this script.
it's also easy to get confused where this script is being used.
This is a nitpick, however, we don't need to refactor it in this PR I think.
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 is slightly confusing, since just passing
-M
to Scala CLI, without any inputs or-cp
will just fail.I know it doesn't fail because
-cp
is passed elsewhere, but this line looks like it should fail, technically.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.
We may want to document this prop clearly somewhere later, once we release this.
I'm also thinking an env variable could potentially be useful?
LEGACY_SCALA_RUNNER=true
or smth.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.
We should probably update coursier ourselves when the new release is up with @bishaboshas's changes in coursier/coursier#2975
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.
cs launch always runs the main class of the artifact you give it, so it makes no difference.
This file was deleted.
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 wonder if it makes sense to keep this comment, or can we just remove it.
it's stated earlier that this is a Scala CLI script.
This file was deleted.