[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Bug#1034378: Allow Percentage Formatting in apt



Hi,

(sry, I seem to be either chronically out of time currently)

On Fri, May 05, 2023 at 04:10:35PM -0000, Emir SARI wrote:
> Hello,I'm attaching a patch that enables translations,

Thanks – but could you perhaps create a merge request on salsa [0]
rather than attaching patches to bug reports as that is easier to
review and less likely to get lost. It also runs out test and all
such, so it gives you and reviewers some direct visible feedback.

[0] https://salsa.debian.org/apt-team/apt/-/merge_requests

> and fixes an issue that creates extra spaces when the percentage sign is prepended to the "Progress: [100%]" string, due to the fixed layout.

I have only very quickly looked at the patch, so only two quick remarks:

The first hunk seems wrong as our 'strprintf' 'prints to a std::string'
given as first argument and doesn't return anything. Meantime, your
change also uses std::printf which means it would print directly to
stdout, which this code shouldn't do either.

In general, I suppose the formatting currently is "Progress: [ 42%]" so
"Progress: [ %42]" is your target?


I think we could go with a "%d%%" string for all (four) cases (which is
my other remark) and assemble the individual strings with e.g.
"Progress: [%s]". The code could format a "%100" first to establish the
maximum length and prepend spaces as needed for the real value.


Oh, and one more thing: Comments in code meant for translators are per
convention prefixed with: "TRANSLATORS: " (see existing usages) and
should try a little harder to convey what a string is used for,
meanwhile a "localize according to your locale settings" could be
said about each and every string…


Best regards

David Kalnischkies

Attachment: signature.asc
Description: PGP signature


Reply to: