Print server banner in store and diff CLI commands - #5026
Conversation
Issue Ericsson#1861 - The server announcement banner was already printed in the login command. This change extends the same behaviour to the store and diff commands (when a remote server is involved). - Extract print_banner() helper in cmd_line_client.py to avoid duplicating the get/decode/log logic - Call print_banner() in store.main() after setup_client() - Call print_banner() in handle_diff_results() after setup_client(), outside the try/except so network errors are not silently swallowed - Extend test_announcement_showing_in_cli to cover store and diff
6e798fc to
6baa10a
Compare
bruntib
left a comment
There was a problem hiding this comment.
Thank you, it's a nice enhancement.
However, I have some suggestions for the implementation:
get_announcement_msg() is called only from print_banner(). Couldn't these two functions be merged?
Also, split_server_url() could also be called from print_banner(), because it's called either way at every occurrence. This way we can save the protocol, host and port parameters. The args.server_url should be enough.
Could we print the log message to LOG? I think, the log=None parameter could be eliminated.
Finally, it would be useful if this banner text would be printed right at the very beginning of each command. The problem is that it gets lost among the lot of other log messages. If you look at handle_login(), the print_banner() call is at the very beginning, but in case of handle_diff_results() it's quite low. Maybe, this could be the first function call after init_logger() in every case.
Please, look at the help page of Codechecker cmd --help. Here you can see, that right now only CodeChecker cmd login and CodeChecker cmd diff are covered. But, there are many other sub-commands, too. A handle_<blabla> function belongs to each. These are also candidates for printing the banner.
Thank you!
Issue #1861 - The server announcement banner was already printed in the login command. This change extends the same behaviour to the store and diff commands (when a remote server is involved).