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
Some more f-strings. #13940
Some more f-strings. #13940
Conversation
/BBox[0 0 %(sidelen)d %(sidelen)d] | ||
/XStep %(sidelen)d | ||
/YStep %(sidelen)d | ||
/BBox[0 0 {sidelen} {sidelen}] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to be defensive and keep the formatting information?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that they're numbers (d
) rather than something else? if anything passing the wrong value seems more likely to happen than passing the wrong type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't see that sidelen = 72
is hard coded just a few lines before. So the risk is actually only in a later code change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's a small risk, but it's much easier to debug a Python TypeError than a broken postscript file. Postscript and pdf viewers don't really show very good error messages.
@anntzer I'm neutral with regard to keeping the formatting, but given @tacaswell's question and @timhoffm's response, I suggest you put it in, at which point I will merge. It (the format specs) can't hurt, right? |
I'd rather leave things as they are (we wouldn't be adding that type info if we were writing that code from scratch, after all) but will include the type info if either @tacaswell or @timhoffm reply here that they really want them :) |
/BBox[0 0 %(sidelen)d %(sidelen)d] | ||
/XStep %(sidelen)d | ||
/YStep %(sidelen)d | ||
/BBox[0 0 {sidelen} {sidelen}] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't see that sidelen = 72
is hard coded just a few lines before. So the risk is actually only in a later code change.
`"..." % locals()` is perhaps *the* construct that f-strings most obviously replace, so let's do that replacement.
I gave in and added type info :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
"..." % locals()
is perhaps the construct that f-strings mostobviously replace, so let's do that replacement.
PR Summary
PR Checklist