[pbs-devel] applied: [PATCH proxmox-backup v3 1/1] proxy/parallel_handler: Improved panic errors with formatted strings

[Laurențiu Leahu-Vlăducu]

I fully agree with you on all points - thanks for the info! :)

On 27.01.25 16:57, Thomas Lamprecht wrote:
> Am 24.01.25 um 16:29 schrieb Laurențiu Leahu-Vlăducu:
>> * Improved errors when panics occur and the panic message is a
>> formatted (not static) string. This worked already for &str literals,
>> but not for Strings.
>>
>> Downcasting to both &str and String is also done by the Rust Standard
>> Library in the default panic handler. See:
>> https://github.com/rust-lang/rust/blob/b605c65b6eb5fa71783f8e26df69975f9f1680ee/library/std/src/panicking.rs#L777
>>
>> * Switched from eprintln! to tracing::error when logging panics in the
>> task scheduler.
> 
> This describes what happens but not why and which visible changes this
> has, or not has, like I asked in my review for v2.
> 
> Background reasoning is important for commit messages, as while a
> summary about "what" changes can be quite nice to have it can almost
> always reconstructed relatively easy from checking the diff. The
> semantic change that affects users and or devs OTOH is harder to
> reconstruct, sometimes even impossible (besides that brains tend to
> forget details, staff is not always around to be able to ask them).
> And while one won't need that information for all commits, one cannot
> really predict for which, so adding a rationale for every non-trivial
> commit–with a rather narrow definition of trivial–is the only way to
> ensure that this information is there when needed; short of time-travel
> ;) Note that one does not need a ton of text, one or two sentences can
> often be enough, sometimes a few short paragraphs tend to help though;
> I'm also fine with always going for relatively long commit messages, a
> short summary at the top would be helpful then though.
> 
> Oh and FWIW, for this change it would have been fine to switch over to
> tracing::error in a separate patch, as it's not directly related to
> better relaying errors from panics.
> 
> The first part above I mentioned on v2, that's on you so to say, the
> latter I did not mention when reviewing on v2, so that's on me, as it's
> not a big change and I did not want to delay it further I now applied
> the patches as is.
> 
> And the reason I went for a relatively big explanation here, which
> actually applies to all devs not just you, is mostly because first,
> you're relatively new at Proxmox thus a bit stricter review with more
> rationale is warranted and second, doing so safes, among others, the
> people writing changelogs and tracking down bugs a ton of time, and I
> tend to do both quite often and know the pain about having to retrace
> all relations and background reasoning from a fresh starts because the
> author did not bother with any commit message, and while it was
> self-explanatory or obvious back then for them, they "interestingly"
> often do not remember when asking about the background in persona; which
> _is_ relatable, as said making errors and forgetting things is human,
> but as the fix is so easy, i.e. just write an actual commit message with
> some actual background, it's nonetheless not a good excuse.  Another
> benefit of writing short summarize is that one needs to reflect on the
> work on a higher level, just like rubber duck programming that can help
> to improve things without another's person's review or sometimes also
> surface some flaws.
> 
> anyhow: applied, thanks!