Repository navigation
Conversation
6e798fc to
6baa10a
Compare
bruntib
left a comment
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!
|
Please, consider the implementation of the suggestions above. Thank you! |
b584c82 to
7d95c1c
Compare
| """ | ||
| Return the file content hash for a file. | ||
| """ | ||
| # amazonq-ignore-next-line |
There was a problem hiding this comment.
Please, remove these "amazonq" comments.
| try: | ||
| protocol, host, port, _ = split_product_url(server_url) | ||
| except Exception: | ||
| protocol, host, port = split_server_url(server_url) |
There was a problem hiding this comment.
What is the purpose of this exception handling?
Also, in what cases does split_server_url() return 3 components?
There was a problem hiding this comment.
split_product_url raises ValueError when given a bare server URL (no product path), so the fallback to split_server_url handles that case; this has been refactored into a _parse_server_url helper to make the intent explicit.
| protocol, host, port = split_server_url(server_url) | ||
| session_token = UserCredentials().get_token(host, port) | ||
| config_client = init_config_client(protocol, host, port, | ||
| session_token) |
There was a problem hiding this comment.
Why do we need to pass the session token here? getNotificationBannerText() doesn't need authentication.
There was a problem hiding this comment.
getNotificationBannerText() doesn't require authentication, so the session token lookup is removed.
2 participants
Fixes #1916 - 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).