-
Notifications
You must be signed in to change notification settings - Fork 78
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
Implement autocorrect to give suggestions for misspelled options #67
Conversation
…t-method # Conflicts: # zio-cli/shared/src/main/scala/zio/cli/Command.scala # zio-cli/shared/src/main/scala/zio/cli/Options.scala # zio-cli/shared/src/main/scala/zio/cli/ParserOptions.scala # zio-cli/shared/src/main/scala/zio/cli/PrimType.scala
@a-morales Amazing work. I see there are some conflicts, can you resolve and I'll do a final pass? |
@@ -0,0 +1,33 @@ | |||
package zio.cli | |||
|
|||
object AutoCorrect { |
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.
Maybe make this package private to cli
by adding private [cli] object AutoCorrect
. Otherwise it will appear in the Scaladoc and users will wonder when to use it.
(args, optionsType) = tuple | ||
tuple <- self.args.validate(args, opts) | ||
(args, argsType) = tuple | ||
_ <- ZIO.when(args.nonEmpty)( |
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 like this. 👍
*/ | ||
final case class ParserOptions( | ||
caseSensitive: Boolean | ||
caseSensitive: Boolean, | ||
autoCorrectLimit: Int |
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 think this is exactly the right place to put this!
However, I would suggest, it's time to rename ParserOptions
to CLIConfig
, because autoCorrectLimit
is not really related to parsing anymore, just configuration data for the library.
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.
By the way, why CLIConfig
and not CliConfig
? Personally I find latter more readable 🙃
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 agree with you.
Then to be consistent, we should change CLIApp
to CliApp
.
@@ -23,5 +25,5 @@ object ParserOptions { | |||
/** | |||
* The default options are case sensitive parsing | |||
*/ | |||
val default: ParserOptions = ParserOptions(false) | |||
val default: ParserOptions = ParserOptions(false, 2) |
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.
Did you experiment with options here? Also, it occurs to me, we should not attempt auto-correct if the option name is too short, e.g. -c
cannot really benefit from auto-correct.
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 played around with this and saw that a value of 2 or 3 is good since from experience with using cli apps most option names are short.
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.
Sounds good.
@@ -197,7 +198,8 @@ object Options { | |||
case Nil => primType.validate(None, opts) | |||
case ::(head, _) => primType.validate(Some(head), opts) | |||
}).bimap(f => p(f), a => tail.drop(1) -> a) | |||
|
|||
case head :: tail if AutoCorrect.levensteinDistance(head, fullname, opts) <= opts.autoCorrectLimit => | |||
IO.fail(p(error(s"""the flag "${head}" is not recognized. Did you mean ${fullname}?"""))) |
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 would capitalize The
because it is the first word in the sentence. Otherwise looks great!
There are a few conflicts with |
…t-method # Conflicts: # zio-cli/shared/src/main/scala/zio/cli/Options.scala
@jdegoes addressed comments and this is ready for a re-review. |
Just one small comment on capitalization, then good to merge. This is going to be an amazing feature! |
Addressed the comment around capitalization of |
@a-morales Awesome work! Congratulations on the merge... |
Implements issue #5 that will show possible options to use if a user misspells an option.