DiamondLightSource / blueapi

Apache License 2.0
5 stars 6 forks source link

Make all CLI user-fault errors give clear messages rather than stack traces #489

Open stan-dot opened 4 months ago

stan-dot commented 4 months ago

extend the #206 work, continuing the conversation with @callumforrester

callumforrester commented 4 months ago

So the argument here is: when something goes wrong in the CLI, in particular when it receives a message from the server saying something has gone wrong, should it print a pretty error message or raise an exception and print a stack trace?

@stan-dot favours the former because it makes the UX cleaner and less cluttered (not to take words out of your mouth, Stan, feel free to correct). My view is that the CLI is primarily a debugging tool and when it breaks, I want as much information as possible. I'm also used to seeing a stack trace and immediately concluding that something has gone wrong.

Will tag others who may have opinions: @DiamondJoseph @DominicOram @abbiemery @coretl

DominicOram commented 4 months ago

Mild agreement with @callumforrester but realistically I don't see anyone not a developer using this so it seems low priority so I don't mind either way

stan-dot commented 4 months ago

CLI is primarily a debugging tool and when it breaks

when users submits wrong data it's not 'application has broken' but 'user submitted wrong data'.

in the Web world this distinction has been decided 30 years ago with HTTP codes 5xx when it's a server error and 4xx when something bad on the client side.

the place where you described my position with insufficient detail is 'when something goes wrong in the CLI'. you put all the cases together while the reasons might be vastly different and the user doesn't need all information, but precisely the information they need to fix this. if an argument was submitted wrong, it is confusing to provide entire stack trace. it's good to encapsulate the error.

CLI is a tool to diagnose the whole system and it should never break. It should give nice information what goes wrong in the system.

muddi900 commented 4 months ago

Would this be a good solution:

 if path.exists():
                 config_loader.use_values_from_yaml(path)
             else:
-                raise FileNotFoundError(f"Cannot find file: {path}")
+                click.echo(f"Cannot find file: {path}")

     ctx.ensure_object(dict)
     loaded_config: ApplicationConfig = config_loader.load()
def listen_to_events(obj: dict) -> None:
             StompMessagingTemplate.autoconfigured(config.stomp)
         )
     else:
-        raise RuntimeError("Message bus needs to be configured")
+        click.echo("Message bus needs to be configured")
stan-dot commented 3 months ago

click itself suggests this: https://click.palletsprojects.com/en/7.x/exceptions/#which-exceptions-exist

muddi900 commented 3 months ago

So this would be more appropriate:

@@ -44,7 +41,7 @@ def main(ctx: click.Context, config: Path | None | tuple[Path, ...]) -> None:
             if path.exists():
                 config_loader.use_values_from_yaml(path)
             else:
-                raise FileNotFoundError(f"Cannot find file: {path}")
+                raise click.FileError(f"Cannot find file: {path}")

     ctx.ensure_object(dict)
     loaded_config: ApplicationConfig = config_loader.load()
@@ -140,7 +137,7 @@ def listen_to_events(obj: dict) -> None:
             StompMessagingTemplate.autoconfigured(config.stomp)
         )
     else:
-        raise RuntimeError("Message bus needs to be configured")
+        click.UsageError("Message bus needs to be configured")

     def on_event(
         context: MessageContext,