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

Be explicit with tuples for %s formatting #1634

Merged
merged 1 commit into from
Nov 18, 2018

Conversation

jeffwidman
Copy link
Collaborator

@jeffwidman jeffwidman commented Nov 13, 2018

Fix #1633

I think I got them all... I would appreciate another pair of eyes to review that I didn't accidentally make mistakes on this.

This is a bit hideous, and definitely overkill, but it's faster than trying to debug TypeErrors in production...

Alternatively, could switch these to .format().

Unfortunate that the lovely new python 3.7 syntax f-strings are not backport-able.


This change is Reviewable

@jeffwidman jeffwidman force-pushed the use-explicit-tuples-for-strings branch from 6aecd54 to 91facfd Compare November 13, 2018 21:48
kafka/metrics/stats/sampled_stat.py Outdated Show resolved Hide resolved
@tvoinarovskyi
Copy link
Collaborator

tvoinarovskyi commented Nov 13, 2018

1 place failed the another 2 eyes test =)

@dpkp
Copy link
Owner

dpkp commented Nov 14, 2018

Looks fine w/ the one modulo issue taras noted. I think format is a better long term solution though.

@jeffwidman jeffwidman force-pushed the use-explicit-tuples-for-strings branch from 91facfd to e2b7102 Compare November 17, 2018 08:43
@jeffwidman
Copy link
Collaborator Author

jeffwidman commented Nov 17, 2018

Good catch @tvoinarovskyi, I fixed.

@jeffwidman jeffwidman force-pushed the use-explicit-tuples-for-strings branch from e2b7102 to 8eb26b6 Compare November 18, 2018 00:38
@jeffwidman jeffwidman merged commit 1d44363 into master Nov 18, 2018
@jeffwidman jeffwidman deleted the use-explicit-tuples-for-strings branch November 18, 2018 08:21
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.

3 participants