Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Placeables: Skip non-placeholders with whitespace #3414

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

unho
Copy link
Member

@unho unho commented Mar 30, 2016

This fix prevents from detecting strings like "% s" to be format placeholders.

Fixes #3368.

@unho unho self-assigned this Mar 30, 2016
@unho unho added the mozilla label Mar 30, 2016
@unho unho added this to the 2.0 milestone Mar 30, 2016
@dwaynebailey
Copy link
Member

@unho mmm you mean except that space in that place is actually valid :(

@unho
Copy link
Member Author

unho commented Mar 30, 2016

@dwaynebailey I mean that we don't want to mark as placeable strings with stuff that are not format placeholders. For example "%s" is a placeholder, but "% s" is not.

@dwaynebailey
Copy link
Member

This placeholder is implementing printf as in man 3 printf, please give reasons for departing from that if you must. I'd be happy if I'd actually get valid arguments from people. And this fix essentially makes it work by making it not work for the space example.

The docs state:
' ' (space) A blank should be left before a positive number produced by a signed conversion (a, A, d, e, E, f, F, g, G, or i).

I agree its a corner case. But until I actually hear arguments about why and documents in the change I convinced that nobody has even bothered to look at the comments in the function or the manual page.

@unho unho removed this from the 2.0 milestone Apr 7, 2016
@unho unho force-pushed the master branch 2 times, most recently from bfec65f to ce2d9e4 Compare May 5, 2016 17:05
This fix prevents from detecting strings like "% s" to be format placeholders.

Fixes translate#3368.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

False positive in FormattingPlaceable
2 participants