Fix #4005 command-line options help is incomplete #4018

fedor-shi · 2018-02-20T10:41:26Z

added default value output
added choices values output

- added default value output - added choices values output

Blaisorblade

Nice, this already looks much closer to what scalac -help does, and it's much simpler than I expected!

I have a few comments on making the code even simpler, but they're small things.

I also checked by hand you signed the CLA (https://www.lightbend.com/contribute/cla/scala/check/ioganegambaputifonguser), don't worry if the PR check for the CLA is stuck.

Blaisorblade · 2018-02-20T12:23:13Z

compiler/src/dotty/tools/dotc/config/Settings.scala

+    def defaultValue: String = implicitly[ClassTag[T]] match {
+      case StringTag => default.asInstanceOf[String]
+      case IntTag => default.asInstanceOf[Int].toString
+      case _ => ""


Both branches could be default.toString, but it seems you want to exclude defaults that aren't String and Int? Could you explain why?

I found that only for these tag classes there are meaningful default values. For example for the -version there is default: Boolean == false, what does not make sense.

Blaisorblade · 2018-02-20T12:26:41Z

compiler/src/dotty/tools/dotc/config/CompilerCommand.scala

+        help.append(s"${format(s.name)} ${s.description}")
+        help.append(formatSettings("Default", s.defaultValue))
+        help.append(formatSettings("Choices", s.legalChoices))
+        help.toString


Since this code already using the s formatter, what about just one expression?
s"${format(s.name)} ${s.description}${formatSettings("Default", s.defaultValue)}${formatSettings("Choices", s.legalChoices)}"? You can put arbitrary expressions inside ${} 🙂

Blaisorblade · 2018-02-20T12:29:38Z

compiler/src/dotty/tools/dotc/config/CompilerCommand.scala

@@ -71,7 +71,14 @@ object CompilerCommand extends DotClass {
      val ss                  = (ctx.settings.allSettings filter cond).toList sortBy (_.name)
      val width               = (ss map (_.name.length)).max
      def format(s: String)   = ("%-" + width + "s") format s
-      def helpStr(s: Setting[_]) = s"${format(s.name)} ${s.description}"
+      def formatSettings(name: String, value: String) = if (value.nonEmpty) format("\n") ++ s"  $name: $value." else ""


Hm, why do we need to use format on \n? I'm probably being slow (or rushed)...
And expressions can go inside the s formatter...
Any chance we can use s"\n${format(name)} ${format(value)}"? That's closer to what we do in helpStr, and hopefully clearer.

I use format("\n") as a hack to start the second line with an empty first column for Default or Choices

…itional settings.

allanrenucci · 2018-02-22T12:38:20Z

compiler/src/dotty/tools/dotc/config/Settings.scala

-    def isDefaultIn(state: SettingsState) = valueIn(state) == default
+    def isDefaultIn(state: SettingsState): Boolean = valueIn(state) == default
+
+    def defaultValue: String = implicitly[ClassTag[T]] match {


I would make it an inner method of def helpStr. Also you don't need a cast here:

def defaultValue(setting: Setting[_]): String = setting.default match { case _: Int | _: String => setting.default.toString case _ => "" }

allanrenucci · 2018-02-22T12:39:23Z

compiler/src/dotty/tools/dotc/config/CompilerCommand.scala

@@ -71,7 +71,15 @@ object CompilerCommand extends DotClass {
      val ss                  = (ctx.settings.allSettings filter cond).toList sortBy (_.name)
      val width               = (ss map (_.name.length)).max
      def format(s: String)   = ("%-" + width + "s") format s
-      def helpStr(s: Setting[_]) = s"${format(s.name)} ${s.description}"
+      def formatSettings(name: String, value: String) = {


You can make it an inner method of def helpStr

Blaisorblade · 2018-02-22T13:22:28Z

compiler/src/dotty/tools/dotc/config/CompilerCommand.scala

      }
+      def helpStr(s: Setting[_]) =
+        s"${format(s.name)} ${s.description} ${formatSettings("Default", s.defaultValue)} ${formatSettings("Choices", s.legalChoices)}"


@ioganegambaputifonguser Thanks for the changes, I think they address most of my suggestions :-)

@Blaisorblade you are welcome. also added comments to make it more clear.

…or default value from cast to match.

Blaisorblade · 2018-02-22T22:39:37Z

compiler/src/dotty/tools/dotc/config/Settings.scala

@@ -88,7 +88,7 @@ object Settings {
        state.update(idx, x.asInstanceOf[T])
    }

-    def isDefaultIn(state: SettingsState) = valueIn(state) == default
+    def isDefaultIn(state: SettingsState): Boolean = valueIn(state) == default



I guess we don’t need to add this type annotation, no? Otherwise, LGTM.

I followed the Boy Scout Rule and added this small thing :) Is it true that the public methods have to have the return type annotation? Please correct me if I am not right.

Oh I see! Fine with me.

fedor-shi · 2018-02-23T14:45:16Z

@allanrenucci done. Could you please take a look?

allanrenucci

Otherwise LGTM. Thanks @ioganegambaputifonguser!

allanrenucci · 2018-02-26T17:14:09Z

compiler/src/dotty/tools/dotc/config/CompilerCommand.scala

@@ -71,7 +73,23 @@ object CompilerCommand extends DotClass {
      val ss                  = (ctx.settings.allSettings filter cond).toList sortBy (_.name)
      val width               = (ss map (_.name.length)).max
      def format(s: String)   = ("%-" + width + "s") format s
-      def helpStr(s: Setting[_]) = s"${format(s.name)} ${s.description}"
+      def helpStr(s: Setting[_]) = {
+        def defaultValue(setting: Setting[_]) = setting.default match {


This doesn't need a parameter:

def defaultValue = s.default match { ...

allanrenucci · 2018-02-26T17:14:54Z

compiler/src/dotty/tools/dotc/config/CompilerCommand.scala

+          else
+            ""
+        }
+        s"${format(s.name)} ${s.description} ${formatSetting("Default", defaultValue(s))} ${formatSetting("Choices", s.legalChoices)}"


Remove extraneous spaces:

s"${format(s.name)} ${s.description}${formatSetting("Default", defaultValue(s))}${formatSetting("Choices", s.legalChoices)}"

…the cmd help string

Fix scala#4005 command-line options help is incomplete

7578c37

- added default value output - added choices values output

fedor-shi mentioned this pull request Feb 20, 2018

Command-line options help is incomplete #4005

Closed

Blaisorblade reviewed Feb 20, 2018

View reviewed changes

Erase the string builder and use the 's' string formatter for all add…

f169cbd

…itional settings.

allanrenucci suggested changes Feb 22, 2018

View reviewed changes

Blaisorblade reviewed Feb 22, 2018

View reviewed changes

Move defaultValue and formatSetting helpers under the helpStr. Refact…

a2a0a0d

…or default value from cast to match.

Blaisorblade reviewed Feb 22, 2018

View reviewed changes

allanrenucci approved these changes Feb 26, 2018

View reviewed changes

Remove unused param from defaultValue call. Remove extra spaces from …

fab4c4f

…the cmd help string

allanrenucci merged commit fcd422a into scala:master Feb 26, 2018

Fix #4005 command-line options help is incomplete #4018

Fix #4005 command-line options help is incomplete #4018

Uh oh!

Conversation

fedor-shi commented Feb 20, 2018

Uh oh!

Blaisorblade left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fedor-shi commented Feb 23, 2018

Uh oh!

allanrenucci left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!