On 5/22/25 08:08, Thomas Lamprecht wrote:
> nit: in general: use the full width for comments (at least 80cc, 100c is
> totally fine
> too).
>
> But most of the comment reads as description for what happens, which is
> relatively
> obvious from reading the code here, e.g. a "log_warn" call isn't exactly
> complex, but
> rather telling on its own already.
>
> While comments can really help, they mostly do when they state the things
> that are
> not already obvious from reading the code in the local context already, like,
> e.g.,
> "distant" effects or assumptions, or if it really is complex and there is not
> a
> good way to simplify the code.
>
> If one want's a comment here it probably would be enough to write something
> like:
>
> # storages can be removed while volumes still exist, check that for better UX.
>
>
> Note that your single comment is not a problem on it's own, but having a lot
> of
> these makes reading code harder and as especially long comments describing the
> code itself, and not the reasons, why's and other such rationale, tend to get
> outdated fast, making it even more confusing to read.
>
> That doesn't mean no comments though, but if, then please lets favor succinct
> comments focusing on background, one or maybe two lines should be enough for
> most
> code that benefits from having one. Exceptions naturally exist, e.g., if you
> write
> some crypto code (please don't, as that's even hard to get right for field
> experts
> with dozens of years of good experience, but just as example) then having more
> comment than code would even be expected.
I opted for wrapping the delete_mountpoint_volume in an eval in this
case, so the comment wasn't necessary anymore, but I'll keep that in
mind. I definitely understand the need for succinct comments. Thanks for
the feedback! Also, I won't be sending patches with crypto code anytime
soon, I promise ;)
>
>>> + my ($storeid) = PVE::Storage::parse_volume_id($volume);
>>> + eval { PVE::Storage::storage_config($storage_cfg, $storeid) };
>>> + my $err = $@;
>>> + PVE::RESTEnvironment::log_warn("failed to delete $volume, $err") if
>>> $err;
>>> +
>>> + if (!$err) {
>>> + delete_mountpoint_volume($storage_cfg, $vmid, $volume);
>>> + }
>>
>> Can we instead just surround the delete_mountpoint_volume() call itself
>> with an eval + printing warning? That also catches other situations
>> where deletion fails and is simpler.
>
> Yeah, that would be nicer. As in
>
> eval {
> foo();
> bar();
> }
> # ... error handling
>
> The bar method won't be called if foo dies.
>
>>
>>> };
>>> PVE::LXC::Config->foreach_volume_full($conf, {include_unused => 1},
>>> $remove_volume);
>>>
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
