-
Notifications
You must be signed in to change notification settings - Fork 23
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
Attempt to make cli messages better #494
Conversation
@@ -51,9 +51,9 @@ object ArgOpts { | |||
case VarToken(name, _) => | |||
validNel(VarRaw(name.value, BottomType)) | |||
case CollectionToken(_, _) => | |||
invalidNel("Array argument in function call not supported. Pass it through JSON.") | |||
invalidNel("Array arguments are currently not supported. Use JSON instead.") |
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.
How to use JSON?
@@ -101,7 +101,8 @@ object ArgOpts { | |||
|
|||
data match { | |||
case None if vars.nonEmpty => | |||
invalidNec("Function have non-literals, so, data should be presented") | |||
// TODO: replace someArguments with actual argument names that where present in the function call | |||
invalidNec("Function call has '(someArguments)' but you didn't provide data or data-path argument when you called 'aqua run'. If you want to pass JSON data as an argument please provide data or data-path argument.") |
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.
--data or --data-path?
@DieMyst could you please print a comma-separated list of vars 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.
plz separate into several lines so it's readable in console
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.
There's a well-known notion of undefined variables. I'd write it like this:
function(a, b, c, d)
^^^
Undefined variables: a, b. You can provide them via --data or --data-path
@@ -135,26 +136,26 @@ object ArgOpts { | |||
|
|||
def dataOpt: Opts[js.Dynamic] = | |||
Opts | |||
.option[String]("data", "Argument map for aqua function in JSON format", "d", "json") | |||
.option[String]("data", "Arguments map in JSON format. Example: aqua run -d '{\"someArg1\": [1,2,3], \"someArg2\": 1}' -f 'someFunc(someArg1, someArg2)' -i aqua -a /dns4/stage.fluence.dev/tcp/19001/wss/p2p/12D3KooWHCJbJKGDfCgHSoCuK9q4STyRnVveqLoXAPBbXHTZx9Cv", "d", "json") |
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.
- Why stage?
- Why docs just for the
--data
argument contains so much data? The full example should go toaqua run
's docs I suppose.
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.
Agree, examples usually represented as a separate section in man pages or help mesages.
} | ||
|
||
def dataFromFileOpt[F[_]: Files: Concurrent]: Opts[F[ValidatedNec[String, js.Dynamic]]] = { | ||
jsonFromFileOpt("data-path", "Path to file with arguments map in JSON format", "p") | ||
jsonFromFileOpt("data-path", "Path to a file with arguments map in JSON format. Example: aqua run -p ./some.json -f 'someFunc(someArg1, someArg2)' -i aqua -a /dns4/stage.fluence.dev/tcp/19001/wss/p2p/12D3KooWHCJbJKGDfCgHSoCuK9q4STyRnVveqLoXAPBbXHTZx9Cv", "p") |
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.
The same questions as for --data
@@ -173,7 +174,8 @@ object ArgOpts { | |||
): ValidatedNec[String, Option[js.Dynamic]] = { | |||
(dataFromArgument, dataFromFile) match { | |||
case (Some(_), Some(_)) => | |||
invalidNec("Pass either data argument or path to file with arguments") | |||
// TODO: maybe allow to use both and simple merge with data argument having higher priority | |||
invalidNec("Please you either data or data-path argument. Don't use both") |
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.
invalidNec("Please you either data or data-path argument. Don't use both") | |
invalidNec("Please you either --data or --data-path. Don't use both") |
@@ -36,7 +36,7 @@ class FuncCompiler[F[_]: Files: AquaIO: Async]( | |||
.map(validNec) | |||
.getOrElse( | |||
Validated.invalidNec[String, FuncArrow]( | |||
s"There is no exported function '${func.name}'. See https://doc.fluence.dev/aqua-book/language/header#export on exporting functions" | |||
s"There is no function '${func.name}'. Check the spelling. Or there is no exported function '${func.name}'. See https://doc.fluence.dev/aqua-book/language/header#export on exporting functions" |
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 find that confusing. Why tell almost same thing twice?
) | ||
} | ||
|
||
def listPeers[F[_]: Applicative]: Command[F[ExitCode]] = | ||
Command( | ||
name = "default_peers", | ||
header = "List addresses of default peers in Fluence network" | ||
header = "List addresses of the default peers in Fluence network" |
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.
why the? It's a concept, no need to have 'the'. The lesser words, the better.
@@ -37,7 +37,7 @@ object DistOpts extends Logging { | |||
|
|||
def srvNameOpt: Opts[String] = | |||
Opts | |||
.option[String]("service", "What service from the config file to deploy", "s") | |||
.option[String]("service", "Service from the config file that we want to deploy", "s") |
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.
Who are 'we'?
.option[String]("service", "Service from the config file that we want to deploy", "s") | |
.option[String]("service", "Service from the config file to deploy", "s") |
@@ -100,7 +100,7 @@ object FuncCaller { | |||
} | |||
|
|||
val TimeoutErrorMessage = | |||
"Function execution failed by timeout. You can increase timeout with '--timeout' option in milliseconds or check if your code can hang while executing." | |||
"Function execution failed by timeout. You can increase the timeout with '--timeout' option in milliseconds or check if your code can hang while executing." |
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.
timeout is a concept, there's no "that timeout" or "another timeout" or "a timeout".
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 here it's better to have 'the' article because we reference something from the previous sentence. It's a particular timeout that we are talking about, not any timeout
@@ -169,7 +169,7 @@ class Runner( | |||
case (Some(s), _: BoxType) if s.function.arg == js.undefined => Validated.validNec(s :: Nil) | |||
case (Some(s), _) if s.function.arg == js.undefined => | |||
Validated.invalidNec( | |||
s"Argument '$n' is undefined, but it's type '$argType' cannot be undefined." | |||
s"Argument '$n' is undefined, but it must be of '$argType' type" |
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.
What is 'undefined' in Aqua? It's a terminology from JS. I'd say "Argument '$n' is missing"
@@ -86,7 +86,7 @@ object AppOpts { | |||
if (exists && isDir) Validated.validNec[String, Path](p) | |||
else | |||
Validated.invalidNec[String, Path]( | |||
s"There is no path ${p.toString} or it is not a directory" | |||
s"'${p.toString}' path does not lead to a directory" |
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.
what does 'lead to directory' mean? I've never seen such terminology, and I guess most our users didn't, too. The previous error was rather standard.
@@ -107,7 +107,7 @@ object AquaCli extends IOApp with Logging { | |||
Validated | |||
.invalidNec( | |||
"Output path should be specified ('--output' or '-o'). " + | |||
"Add '--dry' to check compilation without output" | |||
"Use '--dry' option if you just want to check that the code compiles and you don't need any output" |
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 long and complicated sentence. You should expect that we have users with bad english.
@@ -15,7 +15,7 @@ object FluenceOpts { | |||
val onOpt: Opts[Option[String]] = | |||
AppOpts.wrapWithOption( | |||
Opts | |||
.option[String]("on", "Where function will be run. Default: host_peer_id", "o", "peerId") | |||
.option[String]("on", "peerId of the peer that will be executing the function. Default: host_peer_id", "o", "peerId") |
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.
.option[String]("on", "peerId of the peer that will be executing the function. Default: host_peer_id", "o", "peerId") | |
.option[String]("on", "peerId of the peer that will execute the function. Default: host_peer_id", "o", "peerId") |
@@ -127,7 +127,8 @@ object ArgOpts { | |||
|
|||
data match { | |||
case None if vars.nonEmpty => | |||
invalidNec("Function have non-literals, so, data should be presented") | |||
// TODO: add a list with actual argument names that where present in the function call | |||
invalidNec("Undefined variables. You can provide them via --data or --data-path flags") |
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 say Missing variables
? It seems we use that word in other places
Please check if it compiles (I didn't bother installing scala but maybe I should) and also check if I didn't mess up the meaning. Thanks!