- 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}")
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.
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?
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.
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?
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!)
> 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}