Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

We fixed f-string typos in popular Python repos

highertier.com

Post reply on HN
Print view View original

71–80 of 154 posts

Re: We fixed f-string typos in popular Python repos

#71

Post by mhils »

rikatee wrote: post #68
mhils wrote: post #61

Earlier quoted context omitted.

We've blocked the bot after their script malfunctioned and they opened a second issue with exactly the same text ( https://github.com/mitmproxy/mitmproxy/issues/5286 ).

klyrs was right about the reply from me (a dev behdind Code Review Doctor) being dismissive in the issue. I apologise for that. FWIW my reaction was classic "expectations not meeting reality": weeks of work to do (what I thought) was a mutually beneficial helpful thing. I was naively not expecting non-positive responses and was ill prepared when you raised valid concerns I had not considered. Again, I am working on t…

Thank you - that explanation makes sense, apology accepted. :)

Re: We fixed f-string typos in popular Python repos

#72

Post by IshKebab »

mhils wrote: post #69

We've been one of 666 repos, and I'm not too happy of having our repo used as advertising space. Some thoughts: - I'm happy to receive fix-a-typo PRs from human users. In this case the other side demonstrated that they care by putting in a bit of manual effort, and a small PR often paves the way towards larger contributions. I also know that open source beginners get really excited about their first small contributio…

I feel the same way about those bots that tell you about insignificant security vulnerabilities in some project you abandoned. It's basically spam.

That said, this does seem like it is a bit more useful. As long as they actually read the changes and make sure they aren't false positives. Which I'm guessing they didn't do for 666 repos.

Re: We fixed f-string typos in popular Python repos

#73

Post by chrisshroba »

GeorgeTirebiter wrote: post #65
masklinn wrote: post #57

Earlier quoted context omitted.

> Indeed, if "{value} is bad" can be automatically f-stringed by an external program automatically --- then why can't Python do this automatically -- so we can get rid of the f-string type as a required explicit declaration? Because it would break existing strings containing braces, such as those used with `str.format`, or string.Template, or literal Jinja templates, ...

Yes, my use case was "I always use F-strings" so these other breakages could not occur, by definition. I suppose it's not that much of a problem to run a program to preprocess source and add in the F But.. Python has a history of introducing new features that break old ones. That seems to me a balance between backward compatibility and future goodness. the "from future import auto-fstring" construct could do it... An…

It's not clear to me that folks would want this behavior globally - I personally would not, because sometimes I want to be able to include curly braces in my strings without it being interpolated, and I prefer the current syntax of having that just work. I'm very comfortable with having to add the 'f' to the string syntax to declare that this particular string should be interpolated.

There are other issues you'd have to deal with as well. Would you interpolate non-literal strings (e.g. in the code `print(input())`, if the user inputs the string "{1+1}", what would be printed?)? How would you propose dealing with jinja and other strings that typically contain curly braces that should not be interpolated? This could also be an issue with (potentially long) strings containing lots of random characters: if a '}' showed up in a string somewhere after a '{', even if separated by tens or hundreds of characters, then you'll run into errors. This could be a problem if you're dealing with pseudorandom or base-92 encoded strings, or even just strings representing code (imagine a python library that generates C++ or Java code which has lots of hard coded strings with braces).

I think overall, having to specify that a particular string should be interpolated is a better solution than having to specify that a string should not be interpolated.

Re: We fixed f-string typos in popular Python repos

#74

Post by jrootabega »

bobbiechen wrote: post #21
memco wrote: post #17

The article links to some docs for the logging module here: https://docs.python.org/3/howto/logging.html#optimization asserting that f-strings are less optimal but the docs do not say that they do not optimize our the expression evaluation of f-strings: only that the logging module tried to perform evaluation as late as possible: where is the f-string described as suboptimal? Relatedly the logging optimization sugges…

I was also confused by the expression evaluation thing. Reading between the lines, it seems like logger.debug("hello %s", foo) may be better than logger.debug(f"hello {foo}") in the case when loglevel is higher than debug. In the first version, the final string does not have to be computed, while in the second version, we might construct the string and then do nothing since the loglevel is excluded.

You're also able to add additional log-specific processors to the log record in the first case.

Re: We fixed f-string typos in popular Python repos

#75

Post by rikatee »

malcolmgreaves wrote: post #39

You can also use flake8 to find this, and even more, errors in Python code.

flake8 does not currently support this check, as they are concerned about the false positives from "what if the string it later used in .format(...)"

However, Code Review Doctor is more of a "this MIGHT be a problem. have you considered..." rather than "it wrong"

Re: We fixed f-string typos in popular Python repos

#76

Post by Waterluvian »

vitus wrote: post #13
Waterluvian wrote: post #4

Earlier quoted context omitted.

I expect to see the entire gamut of possible reactions with a sufficient number of bot PRs. But in looking at 10 of them at random, I didn't find a single "negative response." (I don't think ignoring it is invalid or wrong by any means, given there's so many reasons one might not engage in a timely manner, or at all, in the issues section or PRs. I don't monitor my repos issues because I just don't feel interested in…

Some negative reactions: https://github.com/mitmproxy/mitmproxy/issues/5285 https://github.com/Qiskit/qiskit-terra/issues/7981 https://github.com/beetbox/beets/issues/4340 I do think those concerns are legitimate. (I also think more tooling is a good thing!)

Thank you for sharing these links!

Re: We fixed f-string typos in popular Python repos

#77

Post by totony »

johnny_castaway wrote: post #54
HL33tibCe7 wrote: post #9

> > We may be looking too deep into this but it seems like many developers think when string concatenation occurs it’s enough to declare the first string as an f-string and the other strings are turned into f-strings by osmosis. It doesn’t. We’re not suggesting this is the case for all developers that accidentally did this error, but interesting nonetheless. I highly doubt that people believed that f-strings worked t…

This may be also caused by confusing syntax highlighting in some editors, for example in VSCode [1] The variable in the second string gets highlighted (with slightly different color, but still) because it would still work with `str.format()`. GitHub doesn't seem to do this. [1] https://imgur.com/a/9KGWVG0

Yeah I got bitten by the same-color syntax highlighting a few times.

Re: We fixed f-string typos in popular Python repos

#79

Post by b6dybuyv »

IshKebab wrote: post #72
mhils wrote: post #69

We've been one of 666 repos, and I'm not too happy of having our repo used as advertising space. Some thoughts: - I'm happy to receive fix-a-typo PRs from human users. In this case the other side demonstrated that they care by putting in a bit of manual effort, and a small PR often paves the way towards larger contributions. I also know that open source beginners get really excited about their first small contributio…

I feel the same way about those bots that tell you about insignificant security vulnerabilities in some project you abandoned. It's basically spam. That said, this does seem like it is a bit more useful. As long as they actually read the changes and make sure they aren't false positives. Which I'm guessing they didn't do for 666 repos.

> As long as they actually read the changes and make sure they aren't false positives. Which I'm guessing they didn't do for 666 repos.

In the article they say that "really a bot found the problem and made the PR, but really a human developer at Code Review Doctor did triage the issue before the PR was raised)".

Re: We fixed f-string typos in popular Python repos

#80

Post by GeorgeTirebiter »

chrisshroba wrote: post #73
GeorgeTirebiter wrote: post #65

Earlier quoted context omitted.

Yes, my use case was "I always use F-strings" so these other breakages could not occur, by definition. I suppose it's not that much of a problem to run a program to preprocess source and add in the F But.. Python has a history of introducing new features that break old ones. That seems to me a balance between backward compatibility and future goodness. the "from future import auto-fstring" construct could do it... An…

It's not clear to me that folks would want this behavior globally - I personally would not, because sometimes I want to be able to include curly braces in my strings without it being interpolated, and I prefer the current syntax of having that just work. I'm very comfortable with having to add the 'f' to the string syntax to declare that this particular string should be interpolated. There are other issues you'd have…

Well, you could put a N" your string with{} in it" to type-specify that you are not F-string. In fact, WHY are strings not fully specified every time? There is nothing wrong with F"This is also an f-string". After all, strings are just number sequences with special meanings assigned to some values in some circumstances.

Seems to me the problem is similar to wanting 1 be interpreted as an int --- if you want float, you change the syntax (and therefore semantics) to 1. or 1.0

It's always possible to conjure corner-case failure modes; but shouldn't the 'common case' be catered to, more than some base-92 encoded strings?

And, by the by, more 'smarts' can be applied to automatic f-string determination. If "{variable-that-exists} foobar" is seen it could plausibly be converted to an f-string.

This leads into a much longer discussion of how our compilers/interpreters are too stupid today, and need to up their game. But probably not here, not now.... and also, thank you for your observations & comments.

Post reply on HN