[pmg-devel] [PATCH pmg-api v2 1/2] utils: allow specifying plain and/or html for finalize_report()

[Christoph Heiss]

Thanks for the review!

On Mon, Oct 07, 2024 at 06:11:28PM GMT, Stoiko Ivanov wrote:
> Thanks for the quick turn-around on this!
>
> on a quick glance - I think I like the approach (and that you converted
> all call-sites of finalize_report directly) - however one
> pitfall/suggestion inline:
>
> On Mon,  7 Oct 2024 12:32:11 +0200
> Christoph Heiss <c.heiss at proxmox.com> wrote:
[..]
> > --- a/src/PMG/Backup.pm
> > +++ b/src/PMG/Backup.pm
> > @@ -418,7 +418,7 @@ sub send_backup_notification {
> >      my $tt = PMG::Config::get_template_toolkit();
> >
> >      my $mailfrom = "Proxmox Mail Gateway <postmaster>";
> > -    PMG::Utils::finalize_report($tt, 'backup-notification.tt', $vars, $mailfrom, $email);
> > +    PMG::Utils::finalize_report($tt, $vars, $mailfrom, $email, html => 'backup-notification.tt');
> I think using a hashref here would help to cause less confusion in the
> future:
> PMG::Utils::finalize_report($tt, $vars, $mailfrom, $email, { html =>
> 'backup-notification.tt'} ); (but looking through our source am not 100%
> sure if we have a strong rule for this)

Yeah, I think we use both variants pretty equally all around ..

>
> your line above is  syntactically equivalent to:
> PMG::Utils::finalize_report($tt, $vars, $mailfrom, $email, "html",
> 'backup-notification.tt'); see:
> https://perldoc.perl.org/perlop#Comma-Operator which I find confusing
> (especially if I touch this code in the future)
>
> since finalize_report is called from quite a few places - I think having
> those parameters in a hashref would be more robust, but maybe I'm
> overlooking something?
>

No, using a hashref would work too I guess! I also don't have a
particular strong opinion on that, so it's fine. As you say, it's a lot
clearer in any case.

I'll work through it and send a v3 soon.