-
Notifications
You must be signed in to change notification settings - Fork 127
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 more interactive install-home command #186
Conversation
8338428
to
5e416a8
Compare
8bffcd4
to
3c6e447
Compare
modules/cli/src/main/scala/scala/cli/commands/AboutOptions.scala
Outdated
Show resolved
Hide resolved
modules/cli/src/main/scala/scala/cli/commands/InstallHome.scala
Outdated
Show resolved
Hide resolved
modules/integration/src/test/scala/scala/cli/integration/InstallHomeTests.scala
Outdated
Show resolved
Hide resolved
modules/cli/src/main/scala/scala/cli/commands/InstallHome.scala
Outdated
Show resolved
Hide resolved
System.err.println("Abort") | ||
sys.exit(1) | ||
} | ||
else |
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 could handle incorrect response here and ask again
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 want to make something similar to apt
installer. There, if you don't accept operating with passing 'Y', it will abort process and you have to run again.
Downgrade to scala-cli should be very rarely situation and we should help user to avoid this operation. So I think, that passing any other sign then 'Y' should reject this operation.
modules/cli/src/main/scala/scala/cli/commands/InstallHome.scala
Outdated
Show resolved
Hide resolved
modules/cli/src/main/scala/scala/cli/commands/AboutOptions.scala
Outdated
Show resolved
Hide resolved
modules/cli/src/main/scala/scala/cli/commands/InstallHome.scala
Outdated
Show resolved
Hide resolved
modules/integration/src/test/scala/scala/cli/integration/InstallHomeTests.scala
Outdated
Show resolved
Hide resolved
modules/integration/src/test/scala/scala/cli/integration/InstallHomeTests.scala
Outdated
Show resolved
Hide resolved
modules/cli/src/main/scala/scala/cli/commands/InstallHome.scala
Outdated
Show resolved
Hide resolved
modules/cli/src/main/scala/scala/cli/commands/InstallHome.scala
Outdated
Show resolved
Hide resolved
modules/cli/src/main/scala/scala/cli/commands/InstallHome.scala
Outdated
Show resolved
Hide resolved
modules/cli/src/main/scala/scala/cli/commands/InstallHome.scala
Outdated
Show resolved
Hide resolved
dc50e31
to
feed2d3
Compare
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 only change the chmod parameter to u+x
or sth like this. The two other comments may be ignored.
System.err.println( | ||
s"Error: scala-cli is already installed $oldVersion and up-to-date. Downgrade to $newVersion pass -f or --force." | ||
) | ||
sys.exit(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'd prefer not to put exit
in a function named logDowngrade
. Minor issue, can be done in a follow-up PR
) | ||
} | ||
|
||
def runInstallHome(): Unit = { |
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.
Very sophisticated test, took me some time that we build fake scala-cli executables that can print only version and juggle them. Maybe we could comment it (in at all)
modules/cli/src/main/scala/scala/cli/commands/VersionOptions.scala
Outdated
Show resolved
Hide resolved
0c61cb6
to
f447d89
Compare
--force
flag, we install scala-cli without any checking, if scala-cli is up-to-date.