kinode-dao / kinode

Kinode OS runtime
https://kinode.org
Apache License 2.0
34 stars 13 forks source link

scripts: improve error handling #220

Open nick1udwig opened 9 months ago

nick1udwig commented 9 months ago

One specific comment and one general comment.

Specific comment

alias can be used in unexpected ways that would ideally error out differently, e.g.:

Alias can be assigned to any process; only fails when that alias is called

Current behavior:

fake.os > alias echo echo:echo:sys
Tue 1/30 10:01 terminal: alias echo set to echo:echo:sys
fake.os > echo hi
Tue 1/30 10:01 terminal: failed to instantiate script: couldn't find /echo:sys/pkg/scripts.json
fake.os > alias echo app_store:app_store:sys
Tue 1/30 10:01 terminal: alias echo set to app_store:app_store:sys
fake.os > echo
Tue 1/30 10:01 terminal: failed to instantiate script: script not in scripts.json file

Desired behavior: Error when the alias is set and provide non-"nerdview" errors. E.g. in first case it'd be nice to say something like "package echo:sys does not exist" and in the second something like "given process is not a script; can only alias scripts"

Different error print if alias DNE

Current behavior:

fake.os > alias echo
Tue 1/30 09:59 terminal: alias echo removed
fake.os > alias echo
Tue 1/30 09:59 terminal: alias echo removed

Desired behavior: Print alias does not exist if an alias does not exist.

General comment

It'd be nice if scripts printed out usage information if they can't parse args. Some are currently more helpful than others, but in general it'd be great to print usage if given no args. E.g., we use clap for some of the arg parsing: can we get that usage information for those that we do?

Current behavior:

fake.os > m
Tue 1/30 10:03 m: failed to parse args
fake.os > alias
Tue 1/30 10:04 alias: no alias given

FYI @tadad

nick1udwig commented 9 months ago

we use clap for some of the arg parsing: can we get that usage information for those that we do?

See, e.g., https://github.com/kinode-dao/kit/blob/ed92ceb8e3026c865fbf2746dfcbafaaeed4ae9b/src/main.rs#L609 https://docs.rs/clap/latest/clap/struct.Command.html#method.render_usage

nick1udwig commented 8 months ago

234 is a good step towards error handling. However, we should print usage whenever we get bad input, not just for empty input.

If there is a failure case, we need to be providing as much and as accurate as possible actionable information to users. This is a general goal, not specific to this case! If there is an error message, we need to carefully think about what cases the user will see this and if the message we are printing will be informative to them. If it will not be: how can we make it informative to them?

Worst case scenario: it gives them info they can give to one of us to debug.

Best case scenario, and what we should be aiming for always: they can see what they did wrong and correct it.

tadad commented 8 months ago

can you point out something specific? script error handling is pretty good across the board

nick1udwig commented 8 months ago

We should print usage whenever we get bad input, not just for empty input.