Skip to content
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

all: remove \n endings from log calls #223

Merged
merged 1 commit into from
Oct 12, 2018

Conversation

lkundrak
Copy link
Member

@lkundrak lkundrak commented Oct 7, 2018

The extra newlines look bad when logging to the console.

@@ -129,7 +129,7 @@ call_done (GObject *source, GAsyncResult *r, gpointer user_data)
if (!v) {
if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED))
return;
_LOGW ("Failed: %s\n", error->message);
_LOGW ("Failed: %s", error->message);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

while at it, can you please improve the logging messges?

"Failed: %s" is not good. "%s" below even less good.

Optimally, every logging line is unique enough, that (given the sources) one can unambiguously find the line which logged the message. That means, error->message should be part of a longer sentence that is unique among all logging messages. Arguably, "Failed: %s" is currently rather unique. But it also is not very expressive: what failed?

Yes, src/dns/nm-dns-systemd-resolved.c was already pre-existing (not your doing), sorry for pointing this out here. I don't "blame" you personally. Would be great if you already look at these logging messages, that you take the burden of improving them. Thanks.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is "no" a good answer here? Does it block a review?

It's somewhat irrelevant to otherwise (intentionally) mechanical change. Also, it seems to me that a much better approach to adjust the logs to your liking (that I definitely don't object) would be to just do the change -- it could easily be less effort than asking me to do it and I'd be happy to review (esp. these days when I'm generally busier with changing diapers than with changing code).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure.

The extra newlines look bad when logging to the console.

NetworkManager#223
@lkundrak lkundrak merged commit 02958bb into NetworkManager:master Oct 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants