Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Unify 'help' and 'version' commands #454

Merged
merged 5 commits into from
Dec 9, 2021
Merged

Unify 'help' and 'version' commands #454

merged 5 commits into from
Dec 9, 2021

Conversation

tpasternak
Copy link
Contributor

No description provided.

@tpasternak tpasternak force-pushed the unify-help branch 2 times, most recently from 64436a3 to eee829d Compare December 6, 2021 15:17
@tpasternak
Copy link
Contributor Author

closes #259

@tpasternak
Copy link
Contributor Author

@romanowski should we also support single-hyphen command? -help and -version?

@romanowski
Copy link
Member

I think -help works right now. Adding support for -version may make dangerous precedence and we will need to add single-hypen version to all option so I am leaning towards not adding support for -version. @alexarchambault what do you think?

@alexarchambault
Copy link
Contributor

I think -help works right now. Adding support for -version may make dangerous precedence and we will need to add single-hypen version to all option so I am leaning towards not adding support for -version. @alexarchambault what do you think?

I don't really mind… It could be added if we supported --version already, which we don't (only scala-cli version).

@tpasternak
Copy link
Contributor Author

This is quite interesting, because we support single-hyphen commands only in order to keep compatibility with scalac command. But if we added -version it would probably return scala-cli's version, not the underlying scalac's

@tpasternak tpasternak marked this pull request as ready for review December 8, 2021 10:38
alexarchambault
alexarchambault reviewed Dec 8, 2021
View reviewed changes
modules/cli/src/main/scala/scala/cli/commands/DefaultBase.scala Outdated
import scala.build.internal
//import scala.cli.commands.SharedOptions

case class DefaultOptions(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT We could move that to its own file, like the other options.

modules/cli/src/main/scala/scala/cli/commands/DefaultBase.scala Outdated
@@ -3,28 +3,48 @@ package scala.cli.commands
import caseapp._
import caseapp.core.Error

import scala.build.internal
//import scala.cli.commands.SharedOptions
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT

Suggested change
//import scala.cli.commands.SharedOptions

modules/cli/src/main/scala/scala/cli/commands/DefaultBase.scala Outdated
object DefaultOptions {
implicit lazy val parser: Parser[DefaultOptions] = Parser.derive
implicit lazy val help: Help[DefaultOptions] = Help.derive
// implicit lazy val jsonCodec: ReadWriter[DefaultOptions] = macroRW
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// implicit lazy val jsonCodec: ReadWriter[DefaultOptions] = macroRW

modules/cli/src/main/scala/scala/cli/commands/DefaultBase.scala Outdated
case class DefaultOptions(
@Recurse
runOptions: RunOptions = RunOptions(),
version: Option[Boolean] = None
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, I we want to support -version (single dash), I think the following would work

Suggested change
version: Option[Boolean] = None
@Name("-version")
version: Option[Boolean] = None

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why an Option[Boolean]? A simple Boolean should do (defaulting to false).

modules/cli/src/main/scala/scala/cli/commands/DefaultBase.scala Outdated
def run(options: RunOptions, args: RemainingArgs): Unit =
if (anyArgs)
def run(options: DefaultOptions, args: RemainingArgs): Unit =
if (options.version.isDefined) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this remains an Option, we should check its value like

Suggested change
if (options.version.isDefined) {
if (options.version.contains(true)) {

Otherwise, --version=false would make it print the version nonetheless.

modules/cli/src/main/scala/scala/cli/commands/DefaultBase.scala Outdated
def run(options: DefaultOptions, args: RemainingArgs): Unit =
if (options.version.isDefined) {
println(internal.Constants.version)
sys.exit(0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need of the sys.exit it seems.

Suggested change
sys.exit(0)

modules/cli/src/main/scala/scala/cli/commands/HelpCmd.scala Outdated
import caseapp._

import scala.cli.ScalaCli
case class HelpOptions()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just like for the other options, this can be moved to its own file.

modules/cli/src/main/scala/scala/cli/commands/HelpCmd.scala Show resolved Hide resolved
modules/cli/src/main/scala/scala/cli/commands/ScalaCommand.scala Outdated Show resolved Hide resolved
tpasternak and others added 2 commits December 8, 2021 21:33
@tpasternak @alexarchambault
Update modules/cli/src/main/scala/scala/cli/commands/ScalaCommand.scala
c3b5cb9 
Co-authored-by: Alexandre Archambault <alexarchambault@users.noreply.github.com>
@tpasternak
Apply suggestions from review
d4b783c
alexarchambault
alexarchambault reviewed Dec 9, 2021
View reviewed changes
modules/cli/src/main/scala/scala/cli/commands/HelpOptions.scala

import caseapp._

case class HelpOptions()
Copy link
Contributor

@alexarchambault alexarchambault Dec 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
case class HelpOptions()
@HelpMessage("Print help message")
case class HelpOptions()

This should give a description to the help command in the help message, like

Other commands:
  bsp                                       Start BSP server
  export                                    Export current project to sbt or Mill
  help                                      Print help message
  install completions, install-completions  Installs completions into your shell
  setup-ide                                 Generate a BSP file that you can import into your IDE
  shebang                                   This command is an equivalent of `run`, but it changes the way how
  update                                    Update scala-cli - it works only for installation script

modules/integration/src/test/scala/scala/cli/integration/RunTestDefinitions.scala Outdated
Comment on lines 1276 to 1279
pprint.log(os.proc(TestUtil.cli, helpOption).call(
check = false,
mergeErrIntoOut = true
).out.text())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this just for debugging?

Suggested change
pprint.log(os.proc(TestUtil.cli, helpOption).call(
check = false,
mergeErrIntoOut = true
).out.text())

modules/integration/src/test/scala/scala/cli/integration/RunTestDefinitions.scala Outdated
// tests if the format is correct instead of comparing to a version passed via Constants
// in order to catch errors in Mill configuration, too
val versionRegex = ".*\\d+[.]\\d+[.]\\d+.*".r
for { versionOption <- Seq("version", "--version") } {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT

Suggested change
for { versionOption <- Seq("version", "--version") } {
for (versionOption <- Seq("version", "--version")) {

@tpasternak tpasternak merged commit 7928503 into VirtusLab:master Dec 9, 2021
@tpasternak tpasternak deleted the unify-help branch December 9, 2021 10:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexarchambault alexarchambault alexarchambault approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tpasternak @romanowski @alexarchambault