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

Fix formatter of auto-installed logging handler. #102

Merged
merged 1 commit into from
Oct 16, 2017

Conversation

CruzR
Copy link
Contributor

@CruzR CruzR commented Oct 16, 2017

Formatter objects in Python 3.2+ support .format()-style format strings
if constructed with the style parameter set to '{', but that was never
backported to Python 2.7.

Formatter objects in Python 3.2+ support .format()-style format strings
if constructed with the style parameter set to '{', but that was never
backported to Python 2.7.
@wpercy
Copy link
Contributor

wpercy commented Oct 16, 2017

Just to make sure I'm on the right page here, this is a backwards-compatibility fix for Python 2.7, correct?

@CruzR
Copy link
Contributor Author

CruzR commented Oct 16, 2017

No, the old code was also broken for Python 3. By default, Formatter objects expect to be constructed with a %-style format string, even in Python 3. In Python 3.2+, the constructor has an additional parameter called style that can be set to '{' to allow .format-style format strings, but since that was never backported, I went with the %-style format string.

@CruzR
Copy link
Contributor Author

CruzR commented Oct 16, 2017

In case you want to reproduce the issue, run

python -m unittest tests.TestGeneralFileParsing.test_bad_line

with and without this commit. Without it, you should see the literal format string without any variable interpolation being printed to stderr, with it the correct error message about an unreadable line being skipped should appear.

@wpercy
Copy link
Contributor

wpercy commented Oct 16, 2017

Looks good to me. I'll merge it in and see if there are any other issues I can knock out before I deploy to pypi.

@CruzR
Copy link
Contributor Author

CruzR commented Oct 16, 2017

I'm currently trying to figure out whether I can implement VCard 2.1 support, so if you want to hold off a couple more days, you could avoid deploying to pypi twice.

@wpercy
Copy link
Contributor

wpercy commented Oct 16, 2017

Sounds good, thanks for the heads up. I'll hold off til next week.

@wpercy wpercy merged commit c4ae08b into skarim:master Oct 16, 2017
@wpercy wpercy added this to the 0.9.6 milestone Oct 16, 2017
@CruzR
Copy link
Contributor Author

CruzR commented Oct 22, 2017

vCard 2.1 is quite painful, looks like I won't be implementing support for it this week.

@CruzR CruzR deleted the loggerfix branch October 22, 2017 02:16
@wpercy wpercy modified the milestone: 0.9.6 Jul 8, 2018
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.

2 participants