Skip to content

Fix + test for string format with %% and keys #1900

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

Closed
wants to merge 2 commits into from

Conversation

dmoisset
Copy link
Contributor

This fixes #1717 and adds the corresponding test

@ddfisher
Copy link
Collaborator

Thanks! This looks pretty good, but I think it would be a little cleaner to instead not create ConversionSpecifiers if they'd have type % (see parse_conversion_specifiers). Would you mind making that change?

Some care needed for "%*%" specifiers that still consume arguments,
so replacement_checkers still needs the explicit specifier.type != '%'
@dmoisset
Copy link
Contributor Author

I made the change and I think its slightly cleaner. I expected it to be better and also allow me to remove the '%' check from replacement_checkers(), but the parser still needs to produce the ConversionSpecifier for the "%*%" check

@ddfisher
Copy link
Collaborator

Oh yikes -- I wasn't aware of "%*%" and similar. In the presence of those, I actually like your initial commit more: "%%" really is a full-on conversion specifier (and not merely an escape sequence as I'd initially thought), but it just doesn't need a value.

Sorry for the flip-flopping!

@fabianhjr
Copy link
Contributor

fabianhjr commented Jul 19, 2016

Also took a swing at it with PR: #1905

What I did was store each result in result and if it was the %% literal just skip it.

@dmoisset
Copy link
Contributor Author

@ddfisher I created a PR with the old version of the fix, #1907 , feel free to apply to the one you prefer (I still think this one is the smallest and cleaner, but at this point I think it's just time to choose one of them (or the one from @fabianhjr ).

@fabianhjr
Copy link
Contributor

Hello @dmoisset, I think you got an off by one error; The PR is #1908

@gvanrossum
Copy link
Member

gvanrossum commented Jul 19, 2016 via email

@ddfisher
Copy link
Collaborator

Closing in favor of #1908. Thanks!

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

Successfully merging this pull request may close these issues.

Incorrect check on string interpolation
4 participants