Increase of Spammy PRs and PR reviews
As someone who is watching the python/cpython repository, I'm very used to see lots of traffic. But lately there have been a surge of spammy PRs which are about the same, generally very trivial subject but individually fixing each occurrence one by one: - Add coverage to X (tens of them, as separate PRs) - Make `test_xxx` executable with direct invocation (tens of them, as separate PRs) - Lint source with flake8, fix linting errors (stylistic changes) And lots of non-committer PR reviews that only approve. Am I the only one who feels like this is only done to 'grind' commits (get a higher number of commits into the repository, and boast about it)? In the past there were one or two people who would submit typo fixes, but most of them weren't making it continuously. The situation right now feels much worse than those.
On 1/29/22 3:14 AM, Lrupert via Python-Dev wrote:
As someone who is watching the python/cpython repository, I'm very used to see lots of traffic. But lately there have been a surge of spammy PRs which are about the same, generally very trivial subject but individually fixing each occurrence one by one: - Add coverage to X (tens of them, as separate PRs) - Make `test_xxx` executable with direct invocation (tens of them, as separate PRs) - Lint source with flake8, fix linting errors (stylistic changes)
I am aware of the coverage PRs, and I think they are quite useful -- a couple bugs were fixed in one of my modules thanks to that author's efforts to make sure all branches were tested. Likewise, the executable test PRs are useful -- I haven't followed them closely, but at the beginning a few of us asked the author to submit one PR for a handful of related fixes, which makes it easier for reviewers to assess the changes. So those two types are not spam. I am unaware of the stylistic changes -- could you list a few?
And lots of non-committer PR reviews that only approve.
I have seen this. Quite irritating.
In the past there were one or two people who would submit typo fixes, but most of them weren't making it continuously. The situation right now feels much worse than those.
As I said earlier, the coverage and executable tests are legitimate, and I welcome them -- they are helping increase the quality of Python's code. -- ~Ethan~
On Sun, Jan 30, 2022 at 10:39 AM Ethan Furman <ethan@stoneleaf.us> wrote:
And lots of non-committer PR reviews that only approve.
I have seen this. Quite irritating.
We can prohibit approval from non core developers. Do we use this setting already? https://github.blog/2021-11-01-github-keeps-getting-better-for-open-source-m... -- Inada Naoki <songofacandy@gmail.com>
As a non-committer, I want to make a plea for non-committer approval reviews, or at least that they have a place. When asked how outsiders can contribute I frequently see "review open PRs" as a suggested way of contributing to CPython. Not being able to approve PRs that are good would be a barrier to those contributions. Furthermore, I am collaborating with a couple of core devs, it would make collaboration more difficult if I couldn't review their work and say that I thought the changes looked good. I know "drive by approvals" are annoying but I think it is unfortunately part of open source projects. On Sat, Jan 29, 2022, 5:51 PM Inada Naoki <songofacandy@gmail.com> wrote:
On Sun, Jan 30, 2022 at 10:39 AM Ethan Furman <ethan@stoneleaf.us> wrote:
And lots of non-committer PR reviews that only approve.
I have seen this. Quite irritating.
We can prohibit approval from non core developers. Do we use this setting already?
https://github.blog/2021-11-01-github-keeps-getting-better-for-open-source-m...
-- Inada Naoki <songofacandy@gmail.com> _______________________________________________ Python-Dev mailing list -- python-dev@python.org To unsubscribe send an email to python-dev-leave@python.org https://mail.python.org/mailman3/lists/python-dev.python.org/ Message archived at https://mail.python.org/archives/list/python-dev@python.org/message/KHXSE2MS... Code of Conduct: http://python.org/psf/codeofconduct/
On Sun, Jan 30, 2022 at 12:03 PM Ethan Smith <ethan@ethanhs.me> wrote:
As a non-committer, I want to make a plea for non-committer approval reviews, or at least that they have a place. When asked how outsiders can contribute I frequently see "review open PRs" as a suggested way of contributing to CPython. Not being able to approve PRs that are good would be a barrier to those contributions.
Furthermore, I am collaborating with a couple of core devs, it would make collaboration more difficult if I couldn't review their work and say that I thought the changes looked good.
You can still write a review comment, including "LGTM". What you can not is labeling PR as "Approved." So I don't think it makes collaboration difficult. By preventing approval from others, we can easily find PRs approved from core-devs or triage members but not merged yet.
I know "drive by approvals" are annoying but I think it is unfortunately part of open source projects.
Sorry, but I don't think so. -- Inada Naoki <songofacandy@gmail.com>
On Sat, Jan 29, 2022 at 7:38 PM Inada Naoki <songofacandy@gmail.com> wrote:
On Sun, Jan 30, 2022 at 12:03 PM Ethan Smith <ethan@ethanhs.me> wrote:
As a non-committer, I want to make a plea for non-committer approval
reviews, or at least that they have a place. When asked how outsiders can contribute I frequently see "review open PRs" as a suggested way of contributing to CPython. Not being able to approve PRs that are good would be a barrier to those contributions.
Furthermore, I am collaborating with a couple of core devs, it would
make collaboration more difficult if I couldn't review their work and say that I thought the changes looked good.
You can still write a review comment, including "LGTM".
Fair enough, I suppose commenting with "LGTM" works just as well.
What you can not is labeling PR as "Approved." So I don't think it makes collaboration difficult.
By preventing approval from others, we can easily find PRs approved
from core-devs or triage members but not merged yet.
I know "drive by approvals" are annoying but I think it is unfortunately part of open source projects.
Sorry, but I don't think so.
Well, if you disallow drive-by approvals, you will still get drive-by LGTM comments. People seem to believe that adding their approval will expedite a PR merge, for some reason (or want to bump a stale PR in hopes of a quicker merge).
-- Inada Naoki <songofacandy@gmail.com>
A non-core approval changes the label from "awaiting review" to "awaiting core review". I find this useful, and occasionally filter on "awaiting core review" because it indicates that at least one other person found the PR sound / interesting. I would not like this to change - I think high quality reviews from non-core contributors are valuable for the project and are a very quick way for the contributor to get the right kind of attention from core devs. If people spam the approvals (i.e., approve PRs without reviewing them) then the distinction between the labels becomes meaningless, of course. Though I do wonder what the motivation for doing that repeatedly would be. My basic assumption is that people usually try not to make fools of themselves. Note that contributors can still approve a perfect PR - a quality review in this case would include a brief comment indicating that you understand the change and perhaps why you find it valuable. On the matter of spammy PRs - I agree with both Rupert and Ethan (F): trivial PRs can add value, and a large number of them can be annoying. You can add value and be annoying at the same time (my triage work on b.p.o is probably in this category. Thankfully it's clear I'm not after "triage points", because there aren't any). At the end of the day, it doesn't really matter what a contributor's motivation is - it's up to the core devs to decide which PRs are valuable enough to merge, or even to review, and who they enjoy working with. These things tend to sort themselves out on their own. I don't think we need to restrict access for non-core contributors compared to the status quo. What I do think we need is to make it easier for core devs to close issues and PRs. We have a huge pile of open issues and PRs, some of which we know will never happen (stale or otherwise) and we don't close them because it's an unpleasant task to let someone down, and sometimes they argue, and we're volunteers and why should we bother with this emotional labour. Through triage I found many 6 year old issues that, once I refreshed and perhaps put an 'easy' label on, got fixed. The useless issues we don't want to close are obscuring the ones we can and should fix. I'm sure this has been discussed before. My only idea is that we formalize some guidelines/criteria for closing old issues and PRs that make it more of a joint decision of the core devs and less of a personal issue between the core dev and the contributor. I would not suggest a blanket 'close issues/PRs with no activity for X months', as I said I found useful 6 year old issues. It has to be more sophisticated than that. In summary - my view is that rather than making contributors contribute less, we should be more effective in reviewing the contributions, including rejecting those that should be rejected. On Sun, Jan 30, 2022 at 8:06 AM Ethan Smith <ethan@ethanhs.me> wrote:
On Sat, Jan 29, 2022 at 7:38 PM Inada Naoki <songofacandy@gmail.com> wrote:
On Sun, Jan 30, 2022 at 12:03 PM Ethan Smith <ethan@ethanhs.me> wrote:
As a non-committer, I want to make a plea for non-committer approval
reviews, or at least that they have a place. When asked how outsiders can contribute I frequently see "review open PRs" as a suggested way of contributing to CPython. Not being able to approve PRs that are good would be a barrier to those contributions.
Furthermore, I am collaborating with a couple of core devs, it would
make collaboration more difficult if I couldn't review their work and say that I thought the changes looked good.
You can still write a review comment, including "LGTM".
Fair enough, I suppose commenting with "LGTM" works just as well.
What you can not is labeling PR as "Approved." So I don't think it makes collaboration difficult.
By preventing approval from others, we can easily find PRs approved
from core-devs or triage members but not merged yet.
I know "drive by approvals" are annoying but I think it is unfortunately part of open source projects.
Sorry, but I don't think so.
Well, if you disallow drive-by approvals, you will still get drive-by LGTM comments. People seem to believe that adding their approval will expedite a PR merge, for some reason (or want to bump a stale PR in hopes of a quicker merge).
-- Inada Naoki <songofacandy@gmail.com>
_______________________________________________ Python-Dev mailing list -- python-dev@python.org To unsubscribe send an email to python-dev-leave@python.org https://mail.python.org/mailman3/lists/python-dev.python.org/ Message archived at https://mail.python.org/archives/list/python-dev@python.org/message/QJ5QBB4X... Code of Conduct: http://python.org/psf/codeofconduct/
On Sun, Jan 30, 2022 at 7:37 PM Irit Katriel <iritkatriel@googlemail.com> wrote:
If people spam the approvals (i.e., approve PRs without reviewing them) then the distinction between the labels becomes meaningless, of course. Though I do wonder what the motivation for doing that repeatedly would be. My basic assumption is that people usually try not to make fools of themselves.
Some people may do "approve without review" to get attention from core dev. They just want to the issue be fixed soon. This is not so bad. Some people may do "approval without review" to make their "Profile" page richer, because GitHub counts it as a contribution. Creating spam issues or pull requests can be reported as spam very easily. But "approve without review" is hard to be reported as spam. So approving random issue is the most easy way to earn contributions without reported as spam. For example, see this user's contribution. They reviewed 32 pull requests in cpython. It seems they approves random pull requests after someone approved it. https://github.com/raghavthind2005 Of course, approving approved pull requests without review don't change the pull request status. So I can ignore them. I just explain "what the motivation approve without review repeatedly". I don't watch the cpython repository so I am not suffered from spammy approvals. So I have no vote for it. I just mention to an option we have. Regards, -- Inada Naoki <songofacandy@gmail.com>
On 1/30/22 04:45, Inada Naoki wrote:
On Sun, Jan 30, 2022 at 7:37 PM Irit Katriel <iritkatriel@googlemail.com> wrote:
Some people may do "approval without review" to make their "Profile" page richer, because GitHub counts it as a contribution. Creating spam issues or pull requests can be reported as spam very easily. But "approve without review" is hard to be reported as spam. So approving random issue is the most easy way to earn contributions without reported as spam.
Whnever there are metrics, some will find a way to game the system to make theirs look better - this certainly isn't limited to github, or to tech, or in any way a recent thing.
On Sun, Jan 30, 2022 at 8:42 AM Mats Wichmann <mats@wichmann.us> wrote:
On 1/30/22 04:45, Inada Naoki wrote:
On Sun, Jan 30, 2022 at 7:37 PM Irit Katriel <iritkatriel@googlemail.com> wrote:
Some people may do "approval without review" to make their "Profile" page richer, because GitHub counts it as a contribution. Creating spam issues or pull requests can be reported as spam very easily. But "approve without review" is hard to be reported as spam. So approving random issue is the most easy way to earn contributions without reported as spam.
Whnever there are metrics, some will find a way to game the system to make theirs look better - this certainly isn't limited to github, or to tech, or in any way a recent thing.
Certainly true, and I think this is more of a social problem than a technical one. If people are giving out review approvals to get more points, you (where 'you' is a person with some privileges on the repo) can click "dismiss review" and get rid of the noise, at least within that PR. Maybe they still get points for the review, I'm not sure. Taking away the ability for non-core contributors to offer official review approvals to stop people like that only harms the people actually trying to do good work. Gaming the system doesn't end up working well in the end anyway. The first time the gamers try to get a job interview and can't explain how they'd do a code review—something GitHub says they've done hundreds or thousands of times—the whole thing will fail.
Where does it say that a review gives you points? The GitHub blog post I saw about the subject only mentions commits. On Mon, Jan 31, 2022 at 8:16 AM Brian Curtin <brian@python.org> wrote:
On Sun, Jan 30, 2022 at 8:42 AM Mats Wichmann <mats@wichmann.us> wrote:
On 1/30/22 04:45, Inada Naoki wrote:
On Sun, Jan 30, 2022 at 7:37 PM Irit Katriel < iritkatriel@googlemail.com> wrote:
Some people may do "approval without review" to make their "Profile" page richer, because GitHub counts it as a contribution. Creating spam issues or pull requests can be reported as spam very easily. But "approve without review" is hard to be reported as spam. So approving random issue is the most easy way to earn contributions without reported as spam.
Whnever there are metrics, some will find a way to game the system to make theirs look better - this certainly isn't limited to github, or to tech, or in any way a recent thing.
Certainly true, and I think this is more of a social problem than a technical one. If people are giving out review approvals to get more points, you (where 'you' is a person with some privileges on the repo) can click "dismiss review" and get rid of the noise, at least within that PR. Maybe they still get points for the review, I'm not sure. Taking away the ability for non-core contributors to offer official review approvals to stop people like that only harms the people actually trying to do good work.
Gaming the system doesn't end up working well in the end anyway. The first time the gamers try to get a job interview and can't explain how they'd do a code review—something GitHub says they've done hundreds or thousands of times—the whole thing will fail. _______________________________________________ Python-Dev mailing list -- python-dev@python.org To unsubscribe send an email to python-dev-leave@python.org https://mail.python.org/mailman3/lists/python-dev.python.org/ Message archived at https://mail.python.org/archives/list/python-dev@python.org/message/R3YU44XP... Code of Conduct: http://python.org/psf/codeofconduct/
-- --Guido van Rossum (python.org/~guido) *Pronouns: he/him **(why is my pronoun here?)* <http://feministing.com/2015/02/03/how-using-they-as-a-singular-pronoun-can-change-the-world/>
I was using points in a more generic sense, making your "contribution activity overview" look nicer—I wasn't sure if "points" was an actual thing or not, so maybe I'm speaking out of turn. Mine shows 70% of my actions are code review, then issues, commits, and PRs are 10% each. On Mon, Jan 31, 2022 at 9:40 AM Guido van Rossum <guido@python.org> wrote:
Where does it say that a review gives you points? The GitHub blog post I saw about the subject only mentions commits.
On Mon, Jan 31, 2022 at 8:16 AM Brian Curtin <brian@python.org> wrote:
On Sun, Jan 30, 2022 at 8:42 AM Mats Wichmann <mats@wichmann.us> wrote:
On 1/30/22 04:45, Inada Naoki wrote:
On Sun, Jan 30, 2022 at 7:37 PM Irit Katriel < iritkatriel@googlemail.com> wrote:
Some people may do "approval without review" to make their "Profile" page richer, because GitHub counts it as a contribution. Creating spam issues or pull requests can be reported as spam very easily. But "approve without review" is hard to be reported as spam. So approving random issue is the most easy way to earn contributions without reported as spam.
Whnever there are metrics, some will find a way to game the system to make theirs look better - this certainly isn't limited to github, or to tech, or in any way a recent thing.
Certainly true, and I think this is more of a social problem than a technical one. If people are giving out review approvals to get more points, you (where 'you' is a person with some privileges on the repo) can click "dismiss review" and get rid of the noise, at least within that PR. Maybe they still get points for the review, I'm not sure. Taking away the ability for non-core contributors to offer official review approvals to stop people like that only harms the people actually trying to do good work.
Gaming the system doesn't end up working well in the end anyway. The first time the gamers try to get a job interview and can't explain how they'd do a code review—something GitHub says they've done hundreds or thousands of times—the whole thing will fail. _______________________________________________ Python-Dev mailing list -- python-dev@python.org To unsubscribe send an email to python-dev-leave@python.org https://mail.python.org/mailman3/lists/python-dev.python.org/ Message archived at https://mail.python.org/archives/list/python-dev@python.org/message/R3YU44XP... Code of Conduct: http://python.org/psf/codeofconduct/
-- --Guido van Rossum (python.org/~guido) *Pronouns: he/him **(why is my pronoun here?)* <http://feministing.com/2015/02/03/how-using-they-as-a-singular-pronoun-can-change-the-world/>
Ah, now I see the section on GitHub user home pages. Honestly if employers just take a glance at that they get what they deserve. I don't want to worry about this, there are enough real problems. On Mon, Jan 31, 2022 at 8:48 AM Brian Curtin <brian@python.org> wrote:
I was using points in a more generic sense, making your "contribution activity overview" look nicer—I wasn't sure if "points" was an actual thing or not, so maybe I'm speaking out of turn. Mine shows 70% of my actions are code review, then issues, commits, and PRs are 10% each.
On Mon, Jan 31, 2022 at 9:40 AM Guido van Rossum <guido@python.org> wrote:
Where does it say that a review gives you points? The GitHub blog post I saw about the subject only mentions commits.
On Mon, Jan 31, 2022 at 8:16 AM Brian Curtin <brian@python.org> wrote:
On Sun, Jan 30, 2022 at 8:42 AM Mats Wichmann <mats@wichmann.us> wrote:
On 1/30/22 04:45, Inada Naoki wrote:
On Sun, Jan 30, 2022 at 7:37 PM Irit Katriel < iritkatriel@googlemail.com> wrote:
Some people may do "approval without review" to make their "Profile" page richer, because GitHub counts it as a contribution. Creating spam issues or pull requests can be reported as spam very easily. But "approve without review" is hard to be reported as spam. So approving random issue is the most easy way to earn contributions without reported as spam.
Whnever there are metrics, some will find a way to game the system to make theirs look better - this certainly isn't limited to github, or to tech, or in any way a recent thing.
Certainly true, and I think this is more of a social problem than a technical one. If people are giving out review approvals to get more points, you (where 'you' is a person with some privileges on the repo) can click "dismiss review" and get rid of the noise, at least within that PR. Maybe they still get points for the review, I'm not sure. Taking away the ability for non-core contributors to offer official review approvals to stop people like that only harms the people actually trying to do good work.
Gaming the system doesn't end up working well in the end anyway. The first time the gamers try to get a job interview and can't explain how they'd do a code review—something GitHub says they've done hundreds or thousands of times—the whole thing will fail. _______________________________________________ Python-Dev mailing list -- python-dev@python.org To unsubscribe send an email to python-dev-leave@python.org https://mail.python.org/mailman3/lists/python-dev.python.org/ Message archived at https://mail.python.org/archives/list/python-dev@python.org/message/R3YU44XP... Code of Conduct: http://python.org/psf/codeofconduct/
-- --Guido van Rossum (python.org/~guido) *Pronouns: he/him **(why is my pronoun here?)* <http://feministing.com/2015/02/03/how-using-they-as-a-singular-pronoun-can-change-the-world/>
-- --Guido van Rossum (python.org/~guido) *Pronouns: he/him **(why is my pronoun here?)* <http://feministing.com/2015/02/03/how-using-they-as-a-singular-pronoun-can-change-the-world/>
Gaming the system doesn't end up working well in the end anyway. The first time the gamers try to get a job interview and can't explain how they'd do a code review—something GitHub says they've done hundreds or thousands of times—the whole thing will fail.
Observably, it feels like they are doing this for core privileges (if they don't already exist, they are a member of the python org?). Every time I see one of those PRs (e.g add test for X, add delete redundant variables for Y), the author seem to be cc-ing their mentor. This gives a bad impression to others about their intentions (constant contribution of trivial / low quality stuff with little-to-no-gain to achieve a higher number of commits, since it is a visible metric).
Okay, now you might as well state which person you are talking about. Who says their mentor hasn't encouraged them to do this? On Mon, Jan 31, 2022 at 12:32 PM Lrupert via Python-Dev < python-dev@python.org> wrote:
Gaming the system doesn't end up working well in the end anyway. The first time the gamers try to get a job interview and can't explain how they'd do a code review—something GitHub says they've done hundreds or thousands of times—the whole thing will fail.
Observably, it feels like they are doing this for core privileges (if they don't already exist, they are a member of the python org?). Every time I see one of those PRs (e.g add test for X, add delete redundant variables for Y), the author seem to be cc-ing their mentor. This gives a bad impression to others about their intentions (constant contribution of trivial / low quality stuff with little-to-no-gain to achieve a higher number of commits, since it is a visible metric).
_______________________________________________ Python-Dev mailing list -- python-dev@python.org To unsubscribe send an email to python-dev-leave@python.org https://mail.python.org/mailman3/lists/python-dev.python.org/ Message archived at https://mail.python.org/archives/list/python-dev@python.org/message/JZBQ2UYT... Code of Conduct: http://python.org/psf/codeofconduct/
-- --Guido van Rossum (python.org/~guido) *Pronouns: he/him **(why is my pronoun here?)* <http://feministing.com/2015/02/03/how-using-they-as-a-singular-pronoun-can-change-the-world/>
On 1/31/22 8:47 AM, Lrupert via Python-Dev wrote:
This gives a bad impression to others about their intentions (constant contribution of trivial / low quality stuff with little-to-no-gain to achieve a higher number of commits, since it is a visible metric).
Two of us have already commented about the usefulness of those PRs and the fact that they have fixed previously unknown bugs -- those are not trivial, and certainly not low quality. If you have a real concern, show us some examples of these useless PRs, otherwise let it be. -- ~Ethan~
On 1/02/22 5:47 am, Lrupert via Python-Dev wrote:
This gives a bad impression to others about their intentions (constant contribution of trivial / low quality stuff with little-to-no-gain to achieve a higher number of commits, since it is a visible metric).
Or they may just feel that it's better to organise their changes into a number of small, independent commits rather than one large one. I wouldn't be too quick to jump to conclusions about people's motives here. -- Greg
On Mon, 31 Jan 2022 at 23:29, Greg Ewing <greg.ewing@canterbury.ac.nz> wrote:
Or they may just feel that it's better to organise their changes into a number of small, independent commits rather than one large one. I wouldn't be too quick to jump to conclusions about people's motives here.
I've found that highly targeted PRs stand the best chance of getting eyeballs/approvals/merges. It's not just preference - it's good sense. And if folks want to "game" GitHub by spamming commits to an open source project, CPython is a pretty poor choice! My occasional contributions can take weeks or months to land due to the thorough review process and peaks/troughs of core dev time for reviews. Barney
If you feel bad impression, sorry about that. The mentee who cc me is under mentoring period. Since tracking all of mentee’s activity is impossible, I requested him to cc me. This was for checking his labeling is valid or not. Warm regards Dong-hee 2022년 2월 1일 (화) 오전 5:35, Lrupert via Python-Dev <python-dev@python.org>님이 작성:
Gaming the system doesn't end up working well in the end anyway. The first time the gamers try to get a job interview and can't explain how they'd do a code review—something GitHub says they've done hundreds or thousands of times—the whole thing will fail.
Observably, it feels like they are doing this for core privileges (if they don't already exist, they are a member of the python org?). Every time I see one of those PRs (e.g add test for X, add delete redundant variables for Y), the author seem to be cc-ing their mentor. This gives a bad impression to others about their intentions (constant contribution of trivial / low quality stuff with little-to-no-gain to achieve a higher number of commits, since it is a visible metric).
_______________________________________________ Python-Dev mailing list -- python-dev@python.org To unsubscribe send an email to python-dev-leave@python.org https://mail.python.org/mailman3/lists/python-dev.python.org/ Message archived at https://mail.python.org/archives/list/python-dev@python.org/message/JZBQ2UYT... Code of Conduct: http://python.org/psf/codeofconduct/
@Lrupert
- Add coverage to X (tens of them, as separate PRs)
I think I know the PRs you're referring to. For the record, some of the PRs tested hairy code paths in the module I maintain. I would have less confidence backporting bugfixes touching that code if we didn't have those tests (the code paths predate me maintaining the module). Sure, the PRs have overwhelmed me at times. However, I'd rather be overwhelmed with test coverage PRs now than with complaints I broke somebody's code on release day :). Inada Naoki wrote:
Some people may do "approval without review" to make their "Profile" page richer, because GitHub counts it as a contribution.
I noticed the same person you mentioned approving PRs right after I send an approval. It's very confusing for me.
El dom, 30 ene 2022 a las 2:40, Irit Katriel via Python-Dev (< python-dev@python.org>) escribió:
A non-core approval changes the label from "awaiting review" to "awaiting core review".
Interestingly, it also changes if a non-core dev requests changes (e.g., see https://github.com/python/cpython/pull/31021). Should that be considered a bug?
I find this useful, and occasionally filter on "awaiting core review" because it indicates that at least one other person found the PR sound / interesting. I would not like this to change - I think high quality reviews from non-core contributors are valuable for the project and are a very quick way for the contributor to get the right kind of attention from core devs.
If people spam the approvals (i.e., approve PRs without reviewing them) then the distinction between the labels becomes meaningless, of course. Though I do wonder what the motivation for doing that repeatedly would be. My basic assumption is that people usually try not to make fools of themselves.
Note that contributors can still approve a perfect PR - a quality review in this case would include a brief comment indicating that you understand the change and perhaps why you find it valuable.
On the matter of spammy PRs - I agree with both Rupert and Ethan (F): trivial PRs can add value, and a large number of them can be annoying. You can add value and be annoying at the same time (my triage work on b.p.o is probably in this category. Thankfully it's clear I'm not after "triage points", because there aren't any). At the end of the day, it doesn't really matter what a contributor's motivation is - it's up to the core devs to decide which PRs are valuable enough to merge, or even to review, and who they enjoy working with. These things tend to sort themselves out on their own.
I don't think we need to restrict access for non-core contributors compared to the status quo. What I do think we need is to make it easier for core devs to close issues and PRs. We have a huge pile of open issues and PRs, some of which we know will never happen (stale or otherwise) and we don't close them because it's an unpleasant task to let someone down, and sometimes they argue, and we're volunteers and why should we bother with this emotional labour.
Through triage I found many 6 year old issues that, once I refreshed and perhaps put an 'easy' label on, got fixed. The useless issues we don't want to close are obscuring the ones we can and should fix.
I'm sure this has been discussed before. My only idea is that we formalize some guidelines/criteria for closing old issues and PRs that make it more of a joint decision of the core devs and less of a personal issue between the core dev and the contributor. I would not suggest a blanket 'close issues/PRs with no activity for X months', as I said I found useful 6 year old issues. It has to be more sophisticated than that.
Agree, the count of 1.6k open PRs is not a good look for new contributors. We should be OK with closing an issue or PR if we are not going to make the change.
In summary - my view is that rather than making contributors contribute less, we should be more effective in reviewing the contributions, including rejecting those that should be rejected.
On Sun, Jan 30, 2022 at 8:06 AM Ethan Smith <ethan@ethanhs.me> wrote:
On Sat, Jan 29, 2022 at 7:38 PM Inada Naoki <songofacandy@gmail.com> wrote:
On Sun, Jan 30, 2022 at 12:03 PM Ethan Smith <ethan@ethanhs.me> wrote:
As a non-committer, I want to make a plea for non-committer approval
reviews, or at least that they have a place. When asked how outsiders can contribute I frequently see "review open PRs" as a suggested way of contributing to CPython. Not being able to approve PRs that are good would be a barrier to those contributions.
Furthermore, I am collaborating with a couple of core devs, it would
make collaboration more difficult if I couldn't review their work and say that I thought the changes looked good.
You can still write a review comment, including "LGTM".
Fair enough, I suppose commenting with "LGTM" works just as well.
What you can not is labeling PR as "Approved." So I don't think it makes collaboration difficult.
By preventing approval from others, we can easily find PRs approved
from core-devs or triage members but not merged yet.
I know "drive by approvals" are annoying but I think it is unfortunately part of open source projects.
Sorry, but I don't think so.
Well, if you disallow drive-by approvals, you will still get drive-by LGTM comments. People seem to believe that adding their approval will expedite a PR merge, for some reason (or want to bump a stale PR in hopes of a quicker merge).
-- Inada Naoki <songofacandy@gmail.com>
_______________________________________________ Python-Dev mailing list -- python-dev@python.org To unsubscribe send an email to python-dev-leave@python.org https://mail.python.org/mailman3/lists/python-dev.python.org/ Message archived at https://mail.python.org/archives/list/python-dev@python.org/message/QJ5QBB4X... Code of Conduct: http://python.org/psf/codeofconduct/
_______________________________________________ Python-Dev mailing list -- python-dev@python.org To unsubscribe send an email to python-dev-leave@python.org https://mail.python.org/mailman3/lists/python-dev.python.org/ Message archived at https://mail.python.org/archives/list/python-dev@python.org/message/VVYK5RTO... Code of Conduct: http://python.org/psf/codeofconduct/
On Sun, Jan 30, 2022 at 08:36:43AM -0800, Jelle Zijlstra wrote:
Agree, the count of 1.6k open PRs is not a good look for new contributors.
How does that compare to other comparable open source projects? Number of open PRs per KLOC seems like a more useful metric than just the number of open PRs. -- Steve
On Sun, 30 Jan 2022 at 20:36, Steven D'Aprano <steve@pearwood.info> wrote:
On Sun, Jan 30, 2022 at 08:36:43AM -0800, Jelle Zijlstra wrote:
Agree, the count of 1.6k open PRs is not a good look for new contributors.
How does that compare to other comparable open source projects?
How it compares is a separate question from how it looks to someone coming from "outside". This is a common problem in community-supported open-source but it is still a problem whichever way you look at it: most significant open source projects have large numbers of unresolved attempts at improvements. The problem is that an issue/PR tracker becomes meaningless when it's mostly filled with no chance of success suggestions/changes.
Number of open PRs per KLOC seems like a more useful metric than just the number of open PRs.
Maybe but either way it's better if there are fewer unresolved PRs sitting there. As Irit says above it's common (in CPython and also many other projects) that something sits there as unresolved even though it's obvious to the experienced that it will never go anywhere. The difficulty is that closing a PR entails awkwardness that is well described by Irit as "emotional labour". It's immediately easier to leave something to languish than to close it off regardless of what is better in the long term. Taking an alternate approach though: if a community cannot cope with the number of already incoming attempts to make changes then maybe it doesn't really benefit from signalling to new contributors that making changes is easy... -- Oscar
Hi, my name is Nikita and I think that I am the person behind these spammy PRs. Link: https://github.com/python/cpython/pulls/sobolevn First of all, Lrupert, sorry for all the noise and inconvenience I caused to you personally and to other community members! This totally was **not** my intention. You could have reached out to me earlier via email or directly in some PR to share your concerns, maybe we could have this whole situation avoided. Secondly, thanks for letting me know about how others might feel about my work. Feedback is always important. I hope I can learn a lot from this case. But, I can tell you honestly that I've never been to a situation like this before and I don't know exactly how to handle it and what to improve in this specific case. Third, I agree that almost all my PRs were about some trivial things. Mostly because I was reading through typeshed code (which requires looking through multiple CPython modules at a very high level) and test code for these modules where I've spotted quite a lot of details to fix. I cannot obviously judge the quality of my work (except for being ok-ish), so I will leave this part out of scope. The only thing I can say here is that it's a bit sad thread to read on python-dev mailing list, but I will keep my optimism for the future :) So, to sum things up: - I am open to any feedback: if others also think that my work does not bring any value - this is fine! I can totally improve or work on something simpler / some other project. Anyone interested can write me a direct email: mail@sobolevn.me - I am sorry for causing this thread. It is far from being a pleasant or technical read Best Regards, Nikita Sobolev https://github.com/sobolevn
On 1/31/2022 7:31 PM, Nikita Sobolev wrote:
Hi, my name is Nikita and I think that I am the person behind these spammy PRs. Link: https://github.com/python/cpython/pulls/sobolevn
Nikita, I don't know if the OP was responding only to your PRs or others, but I other people have seen truly trivial PRs from other people. Example: just after this thread started, someone opened an issue and a PR with news item to change an IDLE test name from 'test_formated' to 'test_formatted'. (I considered rejecting as 'too trivial'. But since the person has been active on multiple projects in the last year and signed the CLA, I expect to eventually merge the PR.) Reviews consisting only of LGTM are a separate but real issue. I find them useless from unknown newcomers, but do not want any discouragement of real reviews, even to point out a one misspelling or suggest one better wording. In any case, I was one of those who approved the idea that it should be possible to run files with a main clause both with a standard command line and from an editor designed to run a file as 'main'. I also encouraged multiple easily reviewable PRs from you. Please continue. -- Terry Jan Reedy
On Tue, Feb 01, 2022 at 12:35:02AM -0500, Terry Reedy wrote:
On 1/31/2022 7:31 PM, Nikita Sobolev wrote:
Hi, my name is Nikita and I think that I am the person behind these spammy PRs. Link: https://github.com/python/cpython/pulls/sobolevn
I also encouraged multiple easily reviewable PRs from you. Please continue.
Preening a large codebase is not only a good thing, but encourages familiarity: not only with the code, but with the opaque and worst-documentend parts of the software development processes: the maintenance lifecycle. The fact people are assuming bad faith and spilling ink about those getting involved with that extremely-underserved part when contributors are sorely needed is counter-productive. It would be better if more people contributed small changes and thus became low-friction contributors who don't require a lot of mentoring.
Terry Jan Reedy
Martin
Hi, Le 01/02/2022 à 07:25, Martin Dengler a écrit :
The fact people are assuming bad faith and spilling ink about those getting involved with that extremely-underserved part when contributors are sorely > needed is counter-productive.
I disagree, the original message was a good-faith question about how to interpret a pattern. It is healthy to check with the group to discuss if there is a problem and how to deal with it. For pull requests, I have recently seen a few contributors who clearly want to improve things and just have to be guided with our process: when to create a ticket or not, when to add news or not, why we don’t use force pushes, why «consistency» is not a sufficient reason to make sweeping cosmetic changes that do really fix or improve something, etc. Guidance was well received by these contributors with good intentions. I have also noticed drive-by, commentless PR approvals that add zero value (often right after a core dev approval), but are counted on the person’s github profile and can’t be dismissed (because they have no comment, I suppose). I think these are minor annoyances; in these two examples the persons are teenagers, self-described as tech enthusiasts, so they’re probably just following what the github interface encourages without engaging with the culture or the real work. On one of the PR I wrote a message tagging the person to express that reviews from non-core devs can have value, but not if they have zero effort behind them (no reply yet). Regards
Hi, On Tue, Feb 01, 2022 at 10:19:12AM -0500, Éric Araujo wrote:
Hi,
Le 01/02/2022 à 07:25, Martin Dengler a écrit :
The fact people are assuming bad faith and spilling ink about those getting involved with that extremely-underserved part when contributors are sorely > needed is counter-productive.
I disagree, the original message was a good-faith question about how to interpret a pattern. It is healthy to check with the group to discuss if there is a problem and how to deal with it.
I understand what you're saying; if we meet in person I'm happy to discuss further -- but as my original point was that long messages about small, probably-in-good-faith-but-could-be-in-bad-faith actions (as these turned out to be) are (further) wastes of time (net-net), I won't continue.
[snip]
Regards
Regards, Martin
Nikita found a very real (and slightly embarrassing!) bug in a patch I wrote for the enum module a few months back, due to his efforts to improve test coverage. And there is an entire section of the DevGuide devoted to "Improving test coverage", stating that PRs such as the ones Nikita has been filing are valuable.I have seen multiple PRs by other authors closed or criticised because they tried to change things in too many modules at once, or make "broad and sweeping" changes. Nikita, however, has always taken care to make his PRs easy to review, by keeping them small and focused.His mentor has already stated that he specifically asked Nikita to cc him in whenever he filed PRs (and I certainly didn't get a "bad impression about his intentions" from the fact he was cc'ing in his mentor, which seems like a very reasonable thing for a new member of the triage team to do).I'm only a triager (like Nikita), but I really don't see any problem here, personally.Best wishes, Alex -------- Original message --------From: Nikita Sobolev <mail@sobolevn.me> Date: 01/02/2022 04:14 (GMT+00:00) To: python-dev@python.org Subject: [Python-Dev] Re: Increase of Spammy PRs and PR reviews Hi, my name is Nikita and I think that I am the person behind these spammy PRs.Link: https://github.com/python/cpython/pulls/sobolevnFirst of all, Lrupert, sorry for all the noise and inconvenience I caused to you personally and to other community members! This totally was **not** my intention. You could have reached out to me earlier via email or directly in some PR to share your concerns, maybe we could have this whole situation avoided.Secondly, thanks for letting me know about how others might feel about my work. Feedback is always important. I hope I can learn a lot from this case.But, I can tell you honestly that I've never been to a situation like this before and I don't know exactly how to handle it and what to improve in this specific case.Third, I agree that almost all my PRs were about some trivial things. Mostly because I was reading through typeshed code (which requires looking through multiple CPython modules at a very high level) and test code for these modules where I've spotted quite a lot of details to fix.I cannot obviously judge the quality of my work (except for being ok-ish), so I will leave this part out of scope. The only thing I can say here is that it's a bit sad thread to read on python-dev mailing list, but I will keep my optimism for the future :)So, to sum things up:- I am open to any feedback: if others also think that my work does not bring any value - this is fine! I can totally improve or work on something simpler / some other project. Anyone interested can write me a direct email: mail@sobolevn.me- I am sorry for causing this thread. It is far from being a pleasant or technical readBest Regards,Nikita Sobolevhttps://github.com/sobolevn_______________________________________________Py... mailing list -- python-dev@python.orgTo unsubscribe send an email to python-dev-leave@python.orghttps://mail.python.org/mailman3/lists/python-dev.python.org/Message archived at https://mail.python.org/archives/list/python-dev@python.org/message/LF7V3ZAS... of Conduct: http://python.org/psf/codeofconduct/
Am 01.02.22 um 01:31 schrieb Nikita Sobolev:
Hi, my name is Nikita and I think that I am the person behind these spammy PRs. Link: https://github.com/python/cpython/pulls/sobolevn
As a typeshed maintainer, Nikita also "spams" typeshed with PRs. I highly appreciate those PRs, which I am sure take a lot of effort, as they improved the typeshed annotations significantly over the last few months. Besides more substantial PRs, Nikita also submits a lot of PRs that improve and modernize small bits and pieces that needed improvement, but that no one found the time yet to tackle. I am glad that the PRs are fairly small and therefore easy to review. I can't speak to the situation of CPython, though. - Sebastian
účastníci (21)
-
Alex Waygood -
Barney Gale -
Brian Curtin -
Dong-hee Na -
Ethan Furman -
Ethan Smith -
Greg Ewing -
Guido van Rossum -
Inada Naoki -
Irit Katriel -
Jelle Zijlstra -
Ken Jin -
Lrupert -
Martin Dengler -
Mats Wichmann -
Nikita Sobolev -
Oscar Benjamin -
Sebastian Rittau -
Steven D'Aprano -
Terry Reedy -
Éric Araujo