9 ms·

We fixed f-string typos in popular Python repos

139 points by rikatee 4y ago

pabs3 4y ago
Its a shame Code Review Doctor isn't open source, then everyone could install it and use it on any code they write.
hn_saver 4y ago
dewey 4y ago
> For science you can see the reactions here. That link seems to be broken: https://github.com/issues?q=is%3Aissue+author%3Acode-review-doctor+archived%3Afalse+is%3Aclosed https://github.com/issues?q=is%3Aissue+author%3Acode-review-... I was actually surprised to read that people would ignore or be annoyed by a bot raising a valid PR that can be easily merged after a quick glance. What would be the reason for that?
- llbeansandrice 4y ago
  I would never tolerate ads in my commit history. That's ridiculous. It's basically using open source repos as an advertising platform for their static-analysis bot. If they want to offer services, they can reach out to the maintainers. This is different than a human opening a valid PR on a OS repo since the commit message includes an ad and now they're advertising on HN.
- Waterluvian 4y ago
  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 supporting my code. Feel free to fork or ignore!)
  - vitus 4y ago
    Some negative reactions: https://github.com/mitmproxy/mitmproxy/issues/5285 https://github.com/mitmproxy/mitmproxy/issues/5285 https://github.com/Qiskit/qiskit-terra/issues/7981 https://github.com/Qiskit/qiskit-terra/issues/7981 https://github.com/beetbox/beets/issues/4340 https://github.com/beetbox/beets/issues/4340 I do think those concerns are legitimate. (I also think more tooling is a good thing!)
    - dekhn 4y ago
      I looked through all three. The first isn't really a complaint because the bot acted in good faith and found an error. In the second one they complained abiout a missing unsubscribe link (reasonable) and in the third one, the author should update their code so it doesn't create a variable named path, then a non-f-string that includes "{path}". I had to stare at the author's comment that it was a false positive for quite a bit to convince myself they were right.
      - vitus 4y ago
        I will point out that in the first two issues, the repo owners also edited the initial report with something along the lines of "removed ad". I disagree that the first isn't a complaint -- the owner stated that this behavior isn't appreciated but decided to let it slide because the issue was valid. In the third issue, the owner also explicitly stated: "I don't think bots posting unsolicited static analysis results are a good idea." I have no opinions on whether the code should be clearer, but that doesn't change the validity of the reaction.
        dekhn 4y ago
        I do think the question of "how should bots that do static analysis work" is an important one, but in the meantime, people are gonna bot and repo managers are gonna complain.
        klyrs 4y ago
        The bot-account's (apparently human-written) reply of "you're very welcome" to the complaint in the third issue is downright dismissive of the problem and kinda passive aggressive. While it seems that the bot did good work overall, the human(s) handling edge cases need work.
- Forge36 4y ago
  What I've found from doing similar types of changes. 1. It's hard to explain the impact to the application of the current problem. Thus it looks like a theoretical issue 2. Sometimes people rely on the bug for their code to work 3. Surprise work can be poorly received (ie: not the current priority)
- TrickardRixx 4y ago
  Automated checking of potential bugs in f-strings is hard. There are lots of false positives. You can see some discussion around this kind of rule in pylint [0]. At the end of the day, the choice to run automated linting tools on a repo is up to the maintainers. Autogenerating PRs like this is incredibly noisy and comes off to me as a blatant advertisement for their "code review doctor" product. [0] https://github.com/PyCQA/pylint/issues/5039 https://github.com/PyCQA/pylint/issues/5039
  - wbobeirne 4y ago
    > Autogenerating PRs like this The article specifically mentions that they were not auto-generated, "It was also interesting to see the reaction from open source developers to unsolicited pull requests from what looks like a bot (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)"
    - deleted 4y ago
      [deleted]
    - deleted 4y ago
      [deleted]
- zamadatix 4y ago
  In reactions they conveniently left out "false positives we still hadn't weeded out". On top of that it can be annoying to have bots making trivial PRs in their own format when you've got a well defined process for it. Lastly it was basically spamming an ad link for the service at the end of the PR comment - even if the other issues didn't come up it's not always well received to do that. Looking at 1 bot it doesn't sound bad, when you have everyones bot doing this kind of stuff it can quickly become more of a nuisance than a help.
- cinntaile 4y ago
  https://github.com/Qiskit/qiskit-terra/pull/7982 https://github.com/Qiskit/qiskit-terra/pull/7982 That guy was not happy. I do agree that it's basically advertising and that's annoying.
- jjoonathan 4y ago
  You're assuming that the PR is valid, but a maintainer can't make that assumption. They have to do the thankless work to figure out the context and handle the fallout if they get it wrong. Let's look at who wins: * Small benefit to bot creator * Tiny benefit to project * Modest cost to maintainer Waves of low-effort resume-padding commits are already a thing. Not a big problem, but bots clearly have the potential to multiply the small problem into a big problem. I'm still open to the idea that bots could be a net win, because most projects really do have heaps of small simple mistakes lying around. I'm sympathetic to the maintainers though. They always seem to get the short end of the stick.
- VWWHFSfQ 4y ago
  Because this is basically just PR spam
  - dekhn 4y ago
    to me, well-intentioned systems wiht a high true positive rate and low false positive rate are welcome so long as they follow reasonable etiquette and norms, which this group seems to do.
- mhils 4y ago
  In our case OPs bot did not open a PR which could have been merged quickly, but filed an issue instead.
- brandonbloom 4y ago
  In addition to what others have already said, my own random sampling now shows quite a high false positive rate.
bvinc 4y ago
So they checked 666 python repositories and fixed bugs in 69 of them. Interesting choice of numbers.
- defterGoose 4y ago
  ...doing the devil's work.
- racl101 4y ago
  Nice!
- InfiniteRand 4y ago
  Fixing F-strings
- saagarjha 4y ago
  It’s a nice coincidence.
readthenotes1 4y ago
I find it ironic that the article points out that relying on error from humans to find errors is something of a hit or miss proposition and suggests that automating error finding is an appropriate course instead of making it less likely to make the error in the first place. For example, I wonder how many errors would have been found if the definition of a format string was the default? That is, how many times would people have written something like "hello {previously-defined-variable}" and not meant to substitute the value of that previously defined variable at runtime?
- MauranKilom 4y ago
  I don't think this makes sense. Plain strings and format strings are not interchangeable, and using one where the other was meant is probably a bug. Would you expect that a user input like "{secret} please" is interpolated? If so, we hopefully agree that this would blow major security holes into any python script processing untrusted user input. And if not... Why not?
  - noobermin 4y ago
    The assumption I'm thinking they mean is to make formatting default and unformatted not default, for example, how "raw" strings were treated, escaped characters are replaced with the ascii code by default unless the string is raw, signified by an 'r' prefixed in front.
  - asvitkine 4y ago
    If you only make it work with string literals (e.g. generate the underlying formatting logic at parse time), it wouldn't allow arbitrary inputs to be treated as f strings.
  - kgeist 4y ago
    >Would you expect that a user input like "{secret} please" is interpolated? That's basically what the recent log4j security vulnerability was all about. "Helpfully" interpolating logs by default.
  - Sohcahtoa82 4y ago
dattboii 4y ago
HL33tibCe7 4y ago
> > 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 this way. Far more likely is that, for example, the expression started as one line, then got split onto two, or some such similar scenario.
- _8j50 4y ago
  You'd be surprised, people who expect python to be "smart" and "figure it out" might think that way.
  - groestl 4y ago
    Well, it's not completely unlogical. 'a' is str b'a' is bytes f'something' might be a separate f-str-type too? 1 is an int 1.2 is a float (1.2 + 1) is a float
    - GeorgeTirebiter 4y ago
      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? After all, we don't specifically add a type to a number like 42 or 3.14159 --- those are implicitly 'int' and 'float' types. I would use such a feature, as I always use f-strings when formatting.
      - bagels 4y ago
        Backwards compatibility for one. Code existed before fstrings that may use curly braces, and you can currently use curly braces in non fstrings without escaping them. Might also be a performance penalty for always having to run the fstring parser.
      - 4y ago
memco 4y ago
The article links to some docs for the logging module here: https://docs.python.org/3/howto/logging.html#optimization 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 suggests setting: raiseExceptions to false for production logging code: where is that set? On the logger, handler or something else?
- bobbiechen 4y ago
  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.
  - masklinn 4y ago
    That's exactly it. Although this becomes more complicated because printf-style string formatting is not free (though it's the cheapest of all methods save fstrings if I remember correctly), and because python does not support lazy parameters if `foo` is a non-trivial expression odds are good it will far outcost either formatting.
  - jrootabega 4y ago
    You're also able to add additional log-specific processors to the log record in the first case.
  - bbayles 4y ago
    The first is better also because you can do things with loggers other than print out their contents. For example, suppose you had a statement like: logger.debug('Database error: %s', error_message) You probably have a logging handler that did the normal string. But you can also have one that keeps a count of how many `Database error: %s` hits there are (as opposed to `Network error: %s`) there are over time. Doing the string substitution would break this aggregation.
fareesh 4y ago
I like python although I don't use it too often. Would it be unfairly critical of me to say that this is the outcome of a bad design choice? Ideally languages should be designed in a way that a bug like this which is so widespread and easy to create, should be caught via some mechanism, either linting or some part of the process.
- polio 4y ago
  Most developers will require an arsenal of static analysis tools to achieve maximum productivity. Linters are an example of such a tool, but they don't exist as part of the language spec itself, AFAIK.
- noobermin 4y ago
  The f-strings are a recent (may be not so recent now) addition to the language, so all the errors stem from it being "new" where people's reflexes / carefulness hasn't adjusted to them yet. I think in addition to the suggestion for linters, updating IDE/editors to incorporate them would help. Syntax highlighting is the primary reason not terminating strings isn't that common of an error anymore, coloring it differently than a normal string might help (or may be it would make things ugly, I don't know).
- nomel 4y ago
  To play devils advocate, that's a matter of perspective. There's nothing special about "{text}". It's just characters. One could claim that the default analysis should be "that's fine", with the option to make it special available with the "f". ;) But, perhaps you're right, and the total number of bugs would be reduced with f-strings, but that would require making them default back in python 1.0. The linter I use has warnings for things-that-look-like-f-strings on by default. But, some of my projects have f-string like text, so special text to tell the linter to ignore them are required all over the place.
- Too 4y ago
  Compare to Typescript having a very small but significant difference. They use backticks instead of an f-prefix. I think this minor difference eliminates all confusion of whether concatenating f strings and normal strings propagate. Same when you split an existing f-string in two because it became too long, there is no risk to forget a backtick on the second pet, in the same way you would with f-prefix, because if you do the closing tick doesn’t match the opening. Linting the existing f-strings is, as shown by this bot, unfortunately very difficult.
malcolmgreaves 4y ago
You can also use flake8 to find this, and even more, errors in Python code.
- rikatee 4y ago
  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"
f7fg_u-_h 4y ago
> After creating 69 pull requests the reaction ranged from: > Annoyance that a bot with no context on their codebase was raising pull requests. A few accepted the bugs were simple enough for a bot to fix and merged the pull request, but a few closed the pull requests and issues without fixing. Fair enough. Open source developers are busy people and are not being paid to interact with what they think it just a bot. We’re not entitled to their limited time. > Neutral silence. They just merged the PR. > Gratitude. “thanks” or “good bot”. I appreciate their self awareness about responses from maintainers.
bayareabadboy 4y ago
mhils 4y ago
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 contributions, and I'm honestly happy to support that. - In contrast, the marginal effort for bot PRs is ~0. It's very easy to generate a small amount of work for a lot of people, and the nice side effect is that the bot's platform is advertised everywhere. As a maintainer, I have never given consent to this and I have no choice to opt out. We are very happy users of some GitHub bots, but I feel it needs to be an active adoption decision by the maintainer. If you want to pitch me your service you may send me an unsolicited email, but don't use our public space to advertise your product without asking. Edit: I don't want to be too harsh to OP here - at least they pointed out a small but valid issue in our case. I very much appreciate their apology at https://news.ycombinator.com/item?id=31210245 https://news.ycombinator.com/item?id=31210245
- IshKebab 4y ago
  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.
  - b6dybuyv 4y ago
    > 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)".
  - colejohnson66 4y ago
    > I feel the same way about those bots that tell you about insignificant security vulnerabilities in some project you abandoned. It's basically spam. If you "archive" your repos, dependabot and friends won’t bother you. Or, you could just disable security alerts in your repo's settings.
zikohh 4y ago
Can I use code review doctor with gitlab? If not what options do I have?
ggm 4y ago
Maybe catenation of an fString and a string should yield an fString by type promotion? String is morally "any" so it feels to me like a contextual narrowing of type.
- carl_dr 4y ago
  What if you wanted to append a string with braces, could you? (I don’t know Python!) Plus it would lead to subtle security vulns and other bugs. A contrived example : f”{bot} spammed my repo saying” + “touch(‘{path}’) was wrong lol.” Now my path var has been disclosed.
  - bikingbismuth 4y ago
    If you want braces in your f strings you double them up. Ex: n = "hn" print(f"hello {n} your path is {{path}}") Out: hello hn your path is {path}
- SnowflakeOnIce 4y ago
  Strings and f-strings are not separate types in Python, sadly.
mjs7231 4y ago
This was posted on Reddit earlier this week with similar negative responses: https://www.reddit.com/r/Python/comments/ubkvrd/10_of_the_666_most_popular_python_github_repos/ https://www.reddit.com/r/Python/comments/ubkvrd/10_of_the_66...
- rikatee 4y ago
  FWIW, HN is much more positive (while also raising valid points that will be taken into account going forward)
  - snapetom 4y ago
    I'd like to add better technical discussion, too.
baisq 4y ago
The comments on this submission are overwhelmingly negative. Why are the comments on PVS-Studio submissions, on the other hand, generally positive?
safwan 4y ago
f-string does not work with GNU/gettext! It is also a common mistake that people make!
gus_massa 4y ago
I don't mind it's a bot, and I really appreciate that apparently a human made the final review before sending the PR. But I don't like that the tittle of the commit is: > Fix issue probably-meant-fstring found at https://codereview.doctor https://codereview.doctor I expect a more neutral title for a commit, something like > Fix fstring in <name-of-file> Each maintainer/project has their own (weird) rules about titles, and if any other files must log the changes, and regression test, and whatever they like. But I think no maintainer/project expect to see the name of the author in the commit tile.

139 points by rikatee 4y ago

pabs3 4y ago

Its a shame Code Review Doctor isn't open source, then everyone could install it and use it on any code they write.

hn_saver 4y ago

dewey 4y ago

> For science you can see the reactions here. That link seems to be broken: https://github.com/issues?q=is%3Aissue+author%3Acode-review-doctor+archived%3Afalse+is%3Aclosed https://github.com/issues?q=is%3Aissue+author%3Acode-review-... I was actually surprised to read that people would ignore or be annoyed by a bot raising a valid PR that can be easily merged after a quick glance. What would be the reason for that?

llbeansandrice 4y ago
I would never tolerate ads in my commit history. That's ridiculous. It's basically using open source repos as an advertising platform for their static-analysis bot. If they want to offer services, they can reach out to the maintainers. This is different than a human opening a valid PR on a OS repo since the commit message includes an ad and now they're advertising on HN.
Waterluvian 4y ago
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 supporting my code. Feel free to fork or ignore!)
- vitus 4y ago
  Some negative reactions: https://github.com/mitmproxy/mitmproxy/issues/5285 https://github.com/mitmproxy/mitmproxy/issues/5285 https://github.com/Qiskit/qiskit-terra/issues/7981 https://github.com/Qiskit/qiskit-terra/issues/7981 https://github.com/beetbox/beets/issues/4340 https://github.com/beetbox/beets/issues/4340 I do think those concerns are legitimate. (I also think more tooling is a good thing!)
  - dekhn 4y ago
    I looked through all three. The first isn't really a complaint because the bot acted in good faith and found an error. In the second one they complained abiout a missing unsubscribe link (reasonable) and in the third one, the author should update their code so it doesn't create a variable named path, then a non-f-string that includes "{path}". I had to stare at the author's comment that it was a false positive for quite a bit to convince myself they were right.
    - vitus 4y ago
      I will point out that in the first two issues, the repo owners also edited the initial report with something along the lines of "removed ad". I disagree that the first isn't a complaint -- the owner stated that this behavior isn't appreciated but decided to let it slide because the issue was valid. In the third issue, the owner also explicitly stated: "I don't think bots posting unsolicited static analysis results are a good idea." I have no opinions on whether the code should be clearer, but that doesn't change the validity of the reaction.
      - dekhn 4y ago
        I do think the question of "how should bots that do static analysis work" is an important one, but in the meantime, people are gonna bot and repo managers are gonna complain.
      - klyrs 4y ago
        The bot-account's (apparently human-written) reply of "you're very welcome" to the complaint in the third issue is downright dismissive of the problem and kinda passive aggressive. While it seems that the bot did good work overall, the human(s) handling edge cases need work.
Forge36 4y ago
What I've found from doing similar types of changes. 1. It's hard to explain the impact to the application of the current problem. Thus it looks like a theoretical issue 2. Sometimes people rely on the bug for their code to work 3. Surprise work can be poorly received (ie: not the current priority)
TrickardRixx 4y ago
Automated checking of potential bugs in f-strings is hard. There are lots of false positives. You can see some discussion around this kind of rule in pylint [0]. At the end of the day, the choice to run automated linting tools on a repo is up to the maintainers. Autogenerating PRs like this is incredibly noisy and comes off to me as a blatant advertisement for their "code review doctor" product. [0] https://github.com/PyCQA/pylint/issues/5039 https://github.com/PyCQA/pylint/issues/5039
- wbobeirne 4y ago
  > Autogenerating PRs like this The article specifically mentions that they were not auto-generated, "It was also interesting to see the reaction from open source developers to unsolicited pull requests from what looks like a bot (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)"
  - deleted 4y ago
    [deleted]
  - deleted 4y ago
    [deleted]
zamadatix 4y ago
In reactions they conveniently left out "false positives we still hadn't weeded out". On top of that it can be annoying to have bots making trivial PRs in their own format when you've got a well defined process for it. Lastly it was basically spamming an ad link for the service at the end of the PR comment - even if the other issues didn't come up it's not always well received to do that. Looking at 1 bot it doesn't sound bad, when you have everyones bot doing this kind of stuff it can quickly become more of a nuisance than a help.
cinntaile 4y ago
https://github.com/Qiskit/qiskit-terra/pull/7982 https://github.com/Qiskit/qiskit-terra/pull/7982 That guy was not happy. I do agree that it's basically advertising and that's annoying.
jjoonathan 4y ago
You're assuming that the PR is valid, but a maintainer can't make that assumption. They have to do the thankless work to figure out the context and handle the fallout if they get it wrong. Let's look at who wins: * Small benefit to bot creator * Tiny benefit to project * Modest cost to maintainer Waves of low-effort resume-padding commits are already a thing. Not a big problem, but bots clearly have the potential to multiply the small problem into a big problem. I'm still open to the idea that bots could be a net win, because most projects really do have heaps of small simple mistakes lying around. I'm sympathetic to the maintainers though. They always seem to get the short end of the stick.
VWWHFSfQ 4y ago
Because this is basically just PR spam
- dekhn 4y ago
  to me, well-intentioned systems wiht a high true positive rate and low false positive rate are welcome so long as they follow reasonable etiquette and norms, which this group seems to do.
mhils 4y ago
In our case OPs bot did not open a PR which could have been merged quickly, but filed an issue instead.
brandonbloom 4y ago
In addition to what others have already said, my own random sampling now shows quite a high false positive rate.

bvinc 4y ago

So they checked 666 python repositories and fixed bugs in 69 of them. Interesting choice of numbers.

defterGoose 4y ago
...doing the devil's work.
racl101 4y ago
Nice!
InfiniteRand 4y ago
Fixing F-strings
saagarjha 4y ago
It’s a nice coincidence.

readthenotes1 4y ago

I find it ironic that the article points out that relying on error from humans to find errors is something of a hit or miss proposition and suggests that automating error finding is an appropriate course instead of making it less likely to make the error in the first place. For example, I wonder how many errors would have been found if the definition of a format string was the default? That is, how many times would people have written something like "hello {previously-defined-variable}" and not meant to substitute the value of that previously defined variable at runtime?

MauranKilom 4y ago
I don't think this makes sense. Plain strings and format strings are not interchangeable, and using one where the other was meant is probably a bug. Would you expect that a user input like "{secret} please" is interpolated? If so, we hopefully agree that this would blow major security holes into any python script processing untrusted user input. And if not... Why not?
- noobermin 4y ago
  The assumption I'm thinking they mean is to make formatting default and unformatted not default, for example, how "raw" strings were treated, escaped characters are replaced with the ascii code by default unless the string is raw, signified by an 'r' prefixed in front.
- asvitkine 4y ago
  If you only make it work with string literals (e.g. generate the underlying formatting logic at parse time), it wouldn't allow arbitrary inputs to be treated as f strings.
- kgeist 4y ago
  >Would you expect that a user input like "{secret} please" is interpolated? That's basically what the recent log4j security vulnerability was all about. "Helpfully" interpolating logs by default.
- Sohcahtoa82 4y ago

dattboii 4y ago

HL33tibCe7 4y ago

> > 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 this way. Far more likely is that, for example, the expression started as one line, then got split onto two, or some such similar scenario.

_8j50 4y ago
You'd be surprised, people who expect python to be "smart" and "figure it out" might think that way.
- groestl 4y ago
  Well, it's not completely unlogical. 'a' is str b'a' is bytes f'something' might be a separate f-str-type too? 1 is an int 1.2 is a float (1.2 + 1) is a float
  - GeorgeTirebiter 4y ago
    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? After all, we don't specifically add a type to a number like 42 or 3.14159 --- those are implicitly 'int' and 'float' types. I would use such a feature, as I always use f-strings when formatting.
    - bagels 4y ago
      Backwards compatibility for one. Code existed before fstrings that may use curly braces, and you can currently use curly braces in non fstrings without escaping them. Might also be a performance penalty for always having to run the fstring parser.
    - 4y ago

memco 4y ago

The article links to some docs for the logging module here: https://docs.python.org/3/howto/logging.html#optimization 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 suggests setting: raiseExceptions to false for production logging code: where is that set? On the logger, handler or something else?

bobbiechen 4y ago
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.
- masklinn 4y ago
  That's exactly it. Although this becomes more complicated because printf-style string formatting is not free (though it's the cheapest of all methods save fstrings if I remember correctly), and because python does not support lazy parameters if `foo` is a non-trivial expression odds are good it will far outcost either formatting.
- jrootabega 4y ago
  You're also able to add additional log-specific processors to the log record in the first case.
- bbayles 4y ago
  The first is better also because you can do things with loggers other than print out their contents. For example, suppose you had a statement like: logger.debug('Database error: %s', error_message) You probably have a logging handler that did the normal string. But you can also have one that keeps a count of how many `Database error: %s` hits there are (as opposed to `Network error: %s`) there are over time. Doing the string substitution would break this aggregation.

fareesh 4y ago

I like python although I don't use it too often. Would it be unfairly critical of me to say that this is the outcome of a bad design choice? Ideally languages should be designed in a way that a bug like this which is so widespread and easy to create, should be caught via some mechanism, either linting or some part of the process.

polio 4y ago
Most developers will require an arsenal of static analysis tools to achieve maximum productivity. Linters are an example of such a tool, but they don't exist as part of the language spec itself, AFAIK.
noobermin 4y ago
The f-strings are a recent (may be not so recent now) addition to the language, so all the errors stem from it being "new" where people's reflexes / carefulness hasn't adjusted to them yet. I think in addition to the suggestion for linters, updating IDE/editors to incorporate them would help. Syntax highlighting is the primary reason not terminating strings isn't that common of an error anymore, coloring it differently than a normal string might help (or may be it would make things ugly, I don't know).
nomel 4y ago
To play devils advocate, that's a matter of perspective. There's nothing special about "{text}". It's just characters. One could claim that the default analysis should be "that's fine", with the option to make it special available with the "f". ;) But, perhaps you're right, and the total number of bugs would be reduced with f-strings, but that would require making them default back in python 1.0. The linter I use has warnings for things-that-look-like-f-strings on by default. But, some of my projects have f-string like text, so special text to tell the linter to ignore them are required all over the place.
Too 4y ago
Compare to Typescript having a very small but significant difference. They use backticks instead of an f-prefix. I think this minor difference eliminates all confusion of whether concatenating f strings and normal strings propagate. Same when you split an existing f-string in two because it became too long, there is no risk to forget a backtick on the second pet, in the same way you would with f-prefix, because if you do the closing tick doesn’t match the opening. Linting the existing f-strings is, as shown by this bot, unfortunately very difficult.

malcolmgreaves 4y ago

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

rikatee 4y ago
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"

f7fg_u-_h 4y ago

> After creating 69 pull requests the reaction ranged from: > Annoyance that a bot with no context on their codebase was raising pull requests. A few accepted the bugs were simple enough for a bot to fix and merged the pull request, but a few closed the pull requests and issues without fixing. Fair enough. Open source developers are busy people and are not being paid to interact with what they think it just a bot. We’re not entitled to their limited time. > Neutral silence. They just merged the PR. > Gratitude. “thanks” or “good bot”. I appreciate their self awareness about responses from maintainers.

bayareabadboy 4y ago

mhils 4y ago

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 contributions, and I'm honestly happy to support that. - In contrast, the marginal effort for bot PRs is ~0. It's very easy to generate a small amount of work for a lot of people, and the nice side effect is that the bot's platform is advertised everywhere. As a maintainer, I have never given consent to this and I have no choice to opt out. We are very happy users of some GitHub bots, but I feel it needs to be an active adoption decision by the maintainer. If you want to pitch me your service you may send me an unsolicited email, but don't use our public space to advertise your product without asking. Edit: I don't want to be too harsh to OP here - at least they pointed out a small but valid issue in our case. I very much appreciate their apology at https://news.ycombinator.com/item?id=31210245 https://news.ycombinator.com/item?id=31210245

IshKebab 4y ago
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.
- b6dybuyv 4y ago
  > 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)".
- colejohnson66 4y ago
  > I feel the same way about those bots that tell you about insignificant security vulnerabilities in some project you abandoned. It's basically spam. If you "archive" your repos, dependabot and friends won’t bother you. Or, you could just disable security alerts in your repo's settings.

zikohh 4y ago

Can I use code review doctor with gitlab? If not what options do I have?

ggm 4y ago

Maybe catenation of an fString and a string should yield an fString by type promotion? String is morally "any" so it feels to me like a contextual narrowing of type.

carl_dr 4y ago
What if you wanted to append a string with braces, could you? (I don’t know Python!) Plus it would lead to subtle security vulns and other bugs. A contrived example : f”{bot} spammed my repo saying” + “touch(‘{path}’) was wrong lol.” Now my path var has been disclosed.
- bikingbismuth 4y ago
  If you want braces in your f strings you double them up. Ex: n = "hn" print(f"hello {n} your path is {{path}}") Out: hello hn your path is {path}
SnowflakeOnIce 4y ago
Strings and f-strings are not separate types in Python, sadly.

mjs7231 4y ago

This was posted on Reddit earlier this week with similar negative responses: https://www.reddit.com/r/Python/comments/ubkvrd/10_of_the_666_most_popular_python_github_repos/ https://www.reddit.com/r/Python/comments/ubkvrd/10_of_the_66...

rikatee 4y ago
FWIW, HN is much more positive (while also raising valid points that will be taken into account going forward)
- snapetom 4y ago
  I'd like to add better technical discussion, too.

baisq 4y ago

The comments on this submission are overwhelmingly negative. Why are the comments on PVS-Studio submissions, on the other hand, generally positive?

safwan 4y ago

f-string does not work with GNU/gettext! It is also a common mistake that people make!

gus_massa 4y ago

I don't mind it's a bot, and I really appreciate that apparently a human made the final review before sending the PR. But I don't like that the tittle of the commit is: > Fix issue probably-meant-fstring found at https://codereview.doctor https://codereview.doctor I expect a more neutral title for a commit, something like > Fix fstring in <name-of-file> Each maintainer/project has their own (weird) rules about titles, and if any other files must log the changes, and regression test, and whatever they like. But I think no maintainer/project expect to see the name of the author in the commit tile.