Re: svn commit: r568168 - in /spamassassin/trunk/lib/Mail/SpamAssassin: ./ BayesStore/ Conf/ Locker/ Logger/ Plugin/ Util/

[Justin Mason <jm...@jmason.org>]

mmartinec@apache.org writes:
> Author: mmartinec
> Date: Tue Aug 21 08:36:41 2007
> New Revision: 568168


> @@ -860,7 +860,8 @@
>      foreach my $dep (split ' ', $deps) {
>        my $deppri = $pri->{$dep};
>        if ($deppri > $basepri) {
> -        dbg("rules: $rule (pri $basepri) requires $dep (pri $deppri): fixed");
> +        dbg("rules: %s (pri %s) requires %s (pri %s): fixed",
> +            $rule,$basepri, $dep,$deppri);
>          $pri->{$dep} = $basepri;
>        }
>      }

I'm afraid I'm -0.5 on this stuff.

I find %s format strings much harder to read (and use) than simple string
interpolation.  In some cases, of course, they make sense; but for most,
they don't.

Could you revert those changes that add %s usage? (I'm happy enough about
the eval changes, though.)

I see your point (in bug 5589) about %s usage being good for speed, but in
hotspots, we should be using would_log() anyway instead of calling
dbg(...) in the first place.

Feel free to add them in *new* code if you like; just not in the
existing code.

--j.

Re: svn commit: r568168 - in /spamassassin/trunk/lib/Mail/SpamAssassin: ./ BayesStore/ Conf/ Locker/ Logger/ Plugin/ Util/

[Mark Martinec <Ma...@ijs.si>]

> - dbg("rules: $rule (pri $basepri) requires $dep (pri $deppri): fixed");
> + dbg("rules: %s (pri %s) requires %s (pri %s): fixed", 
> +     $rule,$basepri, $dep,$deppri);

> I'm afraid I'm -0.5 on this stuff.
>
> I find %s format strings much harder to read (and use) than simple string
> interpolation.  In some cases, of course, they make sense; but for most,
> they don't.
>
> Could you revert those changes that add %s usage? (I'm happy enough about
> the eval changes, though.)

Ok, will revert the straightforward cases involving mostly scalars.
Yell if you feel I missed any.

  Mark