We fixed f-string typos in popular Python repos(highertier.com) |
We fixed f-string typos in popular Python repos(highertier.com) |
- 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
Now, if the bot catches an actual error and improves the software, the result is obviously net good and the tad of free advertising is deserved. But it can easily feel like a PR campaign paid for with carelessly annexed maintainer time and in quite a few cases, it simply is.
Everyone is out for notoriety and street cred instead of just doing good for the community.
By all means, I am not against having bots identify themselves properly, my point is that "effort from bot PR is ~0", "it advertises their platform" are simply not the right reasons to judge this situation by.
You have a public repository on GitHub. You are free to switch it to private, but otherwise this absolutely illogical. No one needs your consent to submit PRs to and highlight a public repository.
This is equivalent to having a website and then getting angry about linking to it, or putting your artwork up for public viewing and then getting angry at someone pointing out a small tear in the fabric.
Actually, now that I think of it, it’s better comparable to someone bringing in a handheld scanner with a company name on it, scanning the artwork and then pointing out the tear.
Which is still totally fine. You’ve given implied consent by making it available to the public. You have decided to make it possible for the public to view it, criticize it and link to it.
Quote Wikipedia,
> Open-source software (OSS) is computer software that is released under a license in which the copyright holder grants users the rights to use, *study*, change, and distribute the software and its source code to anyone *and for any purpose*
Case closed.
No, it's more like somebody sending to your lab, uninvited, an impersonal inspection bot with another company's branding on it, which doesn't only disclose potential issues to you but advertises them across the whole cyberspace.
And in case of OSS this lab may be my tiny garage where me and friends tinker on stuff.
Choosing to make the results of our passion or work free for all to study and use should not come with a liability of having to deal with hordes of such bots.
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.
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)".
If you "archive" your repos, dependabot and friends won’t bother you.
Or, you could just disable security alerts in your repo's settings.
Your repo is public so you can't prevent people and bots from looking into it and having opinions on it (even public ones).
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.
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.
'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
> Fix issue probably-meant-fstring found at 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.
Relatedly the logging optimization suggests setting: raiseExceptions to false for production logging code: where is that set? On the logger, handler or something else?
logger.debug("hello %s", foo)
logger.debug(f"hello {foo}")
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?
That link seems to be broken: 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?
> 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.
However, Code Review Doctor is more of a "this MIGHT be a problem. have you considered..." rather than "it wrong"
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).
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.
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.
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.
Ex:
n = "hn"
print(f"hello {n} your path is {{path}}")
Out: hello hn your path is {path}
logger.debug('Database error: %s', error_message)
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.
I guess it's implicit in that f-strings, as arguments, will be evaluated before the logging function can even run whereas `debug("...", heavy_obj)` will avoid a potentially expensive `str(heavy_obj)` (or whatever the string conversion warrants.)
As for raiseExceptions, I'm not sure it's for optimization. It looks like an old sanity check for bad logging configurations.
Low value junk like this is not helpful.
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?
That's basically what the recent log4j security vulnerability was all about. "Helpfully" interpolating logs by default.
One thing that it would break is that strings read from files would be treated differently from those in source code, even those read from files that logically “belong” to the application (say config file)
I don’t think that’s an issue, though.
Also, in Swift "\(foo)" does string interpolation. I haven’t seen people complain it leaks data or makes Swift slow (but then, it’s not fast at compiling at all because of its rather complicated type inference)
You can't fix the syntax and standard lib of the language. It is what it is. Similarly, how many bugs would you prevent if Python had compiler support to catch those types of syntax (and type) errors.
Spaces as list separator could also fall into this philosophical question of what makes most sense as string separators. Some times it is super convenient, until you have actual spaces in your string and it becomes a pita.
See also the yaml Norway problem for what happens when implicit goes over explicit.
It generates about the same amount of bugs, if not more, and would also end up with a code-review-doctor suggesting you to use /$ over $. In the end, regardless of syntax, a human always have to make the final call on whether interpolation is wanted or not.
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)"
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.
* Small benefit to bot creator
* Tiny benefit to project
* Modest cost to maintainer
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.
That guy was not happy. I do agree that it's basically advertising and that's annoying.
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.
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)
(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!)
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!)
Did that happen in the example being discussed in this thread?
I think that the claim is not that this leaks data in an absolute sense, but rather that changing the behaviour after people have come to rely on it will leak data from currently well behaving applications.
raise SomeError(
"Explanation... "
"yet more explanation"
)
raise SomeError(
"Explanation... " "yet more explanation"
)
#"\#(expression)"#
That is exactly the point.
I would use such a feature, as I always use f-strings when formatting.
Because it would break existing strings containing braces, such as those used with `str.format`, or string.Template, or literal Jinja templates, ...
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...
And, as for having to run the f-string parser: yes, but only once on static strings, which are most of them.
f-strings, like r-strings, have uses, but, like r-strings, I wouldn't want to replace plain strings with them.
Might also be a performance penalty for always having to run the fstring parser.
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.
> And in case of OSS this lab may be my tiny garage where me and friends tinker on stuff.
That's not OSS. OSS would be leaving the garage door open, putting your garage on Google Maps and freely allowing anyone to walk in and see what you're doing. That's OSS.
Then getting angry about it is what you and OP are doing.
I don't get how your flawed analogy has evolved now. Care to expound?
Edit: I see your edit, thanks. Yes, if we leave garage doors open we still don't welcome these bots, sorry.
Public websites get crawled and indexed hundreds of times per day and sometimes linked to even with criticism. Would you not say this is the same concept?
GitHub isn't your lab. It's Microsoft's lab. (They just rent out space free of charge.)
My point is, we should cherish the culture that enables progress and learning by routinely opening works of passion to free use and contribution. We could cherish it by employing a sense of ethics and adherence to certain protocols of behavior, these don't need to be spelled out but those of us who know better can lead by example. Putting OSS maintainers under undue stress has come under fire before, and this looks like one of those cases.
On the one hand, it's great that we have platforms that innovate and improve and harden over time, but we're also facing a development culture where more and more time is spent servicing package/platform/language/OS changes that have no material impact on our own otherwise-mature projects.
It's worth being judicious about where breaking changes are applied, right?
Did you think this through? What would you treat as a fatal error? How would the compiler know if a particular string is old style code wanting to print some characters between curly braces or new style code wanting to string interpolate a variable?
Python already went through exactly that disaster once before, when they changed the default string type from b””-strings to u””-strings. It took about 20 years for this transition to finally complete.
Also, want to talk Java? Let's not forget that log4j was exploited precisely because of implicit string conversions.
Implicit f-strings are a really bad idea.
Too bad we can't go back in time to 1996 or so.
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.
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.
It is: normal strings are the common case.
One obvious and dangerous application of these "smarts" is when people expect curly braces to be treated as string literals. What if a string contained an example f-string that contained {sensitive-server-information}? It's reasonable to expect that code like that wouldn't later become vulnerable to injection attacks.
Unless you're using any dependency at all, including the standard library.
> But.. Python has a history of introducing new features that break old ones.
It doesn't tho.
> That seems to me a balance between backward compatibility and future goodness.
That assumes "everything is an fstring" is considered "future goodness", which I'm not sure is a widely shared view.
> the "from future import auto-fstring" construct could do it...
That seems unlikely as the __future__ pseudo-package has generally been used to opt into hopefully future behaviour.
Given the Python 3 experience, somehow, I don't see "let's make all string literals into fstrings" happen any time soon, but hey feel free to create a PEP proposing that.
Alternatively, create your own import hook which does this swap before handing the module off of to the compiler.
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 that and sorry I was passive aggressive to you.
As to 'injection' attacks -- I have no sympathy for somebody who takes user input, does not check it, and throws it at SQL. All user input should be checked 7 ways from Sunday before it's even passed to a part of the program that's supposed to deal with said input.
From my perspective, I'm just trying to say "there has got to be a better way". yes, we can survive, but -- as the OP shows -- a bunch of auto-fixed bugs went away. Why were the bugs there in the first place?
I have a saying that programmers spend too much time debugging; and if they would just not put the bugs in, in the first place, debugging could be reduced. To do that, you need help from your language. Other things also matter, sure. But give me constructs that make it hard for me to screw up.
I do want to point out that only interpolating f-strings containing variables that exist is actually more likely to cause bugs because a string "{foo}" may be fine at one point, but then down the road another programmer happens to declare a global variable foo somewhere completely different, thus causing this string to change. The more important issue, though, is that f-strings contain expressions, not just variables, so you can do things like f'{1+1}' or f'There are {len(os.environ)} env vars defined.'. It's not clear what special cases for expressions inside the braces would magically become f-strings and which would not.