Cleanup f-strings, logging and formats

[sveinse]

This PR changes all occurrences of % formatting and str.format() to f-strings.

Please review, as it is easy to sneak in a typo in these changes.

[christiansandberg]

Looked through most of them. 👍

[acolomb]

Only checked on the surface, could use some more time when back at a PC keyboard.

There's some semantic differences you should make sure are intended. And some opportunities to fix inconsistent formats while already touching the lines.

[sveinse]

And some opportunities to fix inconsistent formats while already touching the lines.

Initially I had planned to make this a like-for-like change to ensure no changed behavior. However, if it's wanted (it seems it is) we can fix inconsistencies.

[sveinse]

@acolomb thank you for very valuable feedback 👌 . I will review and post an update.

[sveinse]

I made the liberty to merge PR #442 into this PR and close 442. The PRs did get into each other's feet.

[acolomb]

It's a lot to review, but I think I've seen all and marked additional fixes spotted.

[sveinse]

It's a lot to review, but I think I've seen all and marked additional fixes spotted.

Thank you for the review. Before diving into the comments: I see that we tend to think "while at this, why can't just fix...". I'm worried that by fixing this, we mix the PR that makes no change to behavior with improvements that does. Do we care about that here? Other projects I've been involved with wants to keep them separate.

[acolomb]

In general, yes. No-op formatting or internal changes should be separated from feature work. But in this case, it'll get ridiculous to touch the same line in two PRs when it's only cleanup. Especially when there would be an intermediate step (e.g. changing to f-string first, then adjusting for hex output) which no-one cares about.

[acolomb]

I think this is good to go. Maybe one last look when I'm less tired. There's two unresolved conversations left, but only with partly unrelated questions, so they shouldn't prevent this from going over the finish line.

[acolomb]

Two more small issues that I'd like your opinion on. @sveinse

[sveinse]

@acolomb I didn't see your latest replies about pretty_index, but as you can see, my latest commit contains a wholesale change to pretty_index for consistency. I like that we control how index:subindex is printed in one spot. I don't think the overhead is that big (nothing we do here in canopen really is).

But reading the comments, do we want to revert this one?

[acolomb]

But reading the comments, do we want to revert this one?

I'd vote for reverting, yes. Sorry for the confusion. I just misread your "outweighs the downsides" comment.

Note that conditional evaluation of the formatting is not really the only benefit from using the %-formatting for logging. The biggest advantage I see is that logging.Handler instances get these values passed in as distinct objects, thus can do with them whatever is necessary. Such as translating the message separately or dumping them in some other machine-readable format like JSON, ...

[sveinse]

@acolomb I have reverted the last commit and resolved the comments regarding formatting. As far as I can tell, I think I have completed all follow-up tasks, wouldn't you agree?

[acolomb]

Sorry for the back and forth on the last two changes. I simply saw the index (int) being formatted as string and wanted consistency with many other places where we use 0x%04X formatting in logging calls. Calling for pretty_index() usage here feels overkill as discussed, and as you noticed yourself, there are many other places where that could be applied as well then.

Let's just harmonize on printing in hex in these two cases (like elsewhere) and call it good.

[acolomb]

Thank you very much for your patience and diligence in this cleanup effort! Nice to see people attending to detail and working to reduce our technical debt. 🥇