Bug fix 3374: Address pofilter -tprintf problems

[asyschikov]

Fixes #3374.

1. Add pipe brackets to printf regex to match boost format specification
2. Add space to list of flags in prinf regex to match classical printf specification so that check will catch '% s'
3. Check that all percent signs are either paired (%%) or part of printf placeholder, otherwise fail check.
4. Tests to cover added changes

[unho]

Can you please add tests for enclosing params in pipes?

[unho]

If no tests are added for this feature I prefer to leave it out for now. If tests are added I prefer tests and this particular change in the regex to be put in a separate PR.

[asyschikov]

@unho there was actually a test for it:

assert passes(stdchecker.printf, "(x,y) = (%|+5|,%|+5|)", "(x,y) = (%|+5|,%|+5|)")`

although that test tested nothing because pipe-brackets were not incorporated in regex, so according to current version of checker this string doesn't have any params. After I implemented a rule "no lone percent signs" this test started to rightfully fail that's why I had to add pipe brackets. So that's why this can't be separated into two PRs. I just added test for this feature: asyschikov@bbd102e

[unho]

Thanks.

[unho]

Please put a reference on the commit message about it fixing the issue, like Fixes #3374, so the issue is autoclosed when we merge the PR.

[unho]

I am wondering if "unmatched percent sign" should actually be a separate check. If I am not wrong current code for "printf" check doesn't detect any error on strings like "10% something", so it would be ok to convert this in a different check.

@dwaynebailey Any input on this?

[asyschikov]

@unho it is up to you of course, but printf function (and python analog) will treat %d% something as 2 placeholders:

>>> '%d% something' % (10, )
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: not enough arguments for format string
>>> '%d% something' % (10, 'asd')
'10asdomething'

[unho]

@unho it is up to you of course, but printf function (and python analog) will treat %d% something as 2 placeholders:

As I said the error is not some printf placeholder being malformed, it is about lone percent signs since it can be either an unescaped percent or a malformed printf placeholder. Thus my suggestion of adding a separate check for it since that way we can report the specific problem "The percent sign is either incorrectly escaped or it is part of a malformed printf placeholder".

Please do fix the other raised issues.

[asyschikov]

@unho right - error is not with particular placeholder, problem with the string as a whole because it will break printf/python formatting. And I added specific error explaining the problem (and made line shorter) cb527b9

[friedelwolff]

Just a comment now that I see this:

This kind of thing is why we need to move towards parameterising placeables in the long run - passing information about what we should be looking for. If we know something is a text file containing Markdown, then we don't do a printf check. If it is python-format, we activate the relevant ones. If it is c-format, we can enable the right one. This will reduce false matches, but also make things faster. This is important for me, since the placeables are slowing things down in Virtaal, so we can't just keep adding ones that will mostly not be used.

Sorry for hijacking this pull request for my comment. I don't know where we discuss what these days :-)

[unho]

@friedelwolff Do you mind create an issue for your valuable input?

[phlax]

@friedelwolff github is defo good for discussing specific issues - but for more immediate discussion we are generally on https://gitter.im/translate/dev

[unho]

@friedelwolff Created issue #3412 to track what you suggested in your last comment.

[unho]

I did some research and here are some timings:

Before	After
26,10	31,69
26,58	31,30
26,49	31,37
26,39	31,92
26,43	31,70

So average times are:

Before	After
26,40	31,60

Which means that with these changes the printf check takes 19.7% more time with respect to master.

Given that I wonder if we should consider adding instead a separate check to begin work towards #3412. I am putting this on hold for now until we figure out which is the best way to proceed since we are working towards releasing a final 2.0 TTK release.