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~

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 wrote:

On Sun, Jan 30, 2022 at 10:39 AM Ethan Furman 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 _______________________________________________ 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 Sat, Jan 29, 2022 at 7:38 PM Inada Naoki wrote:

On Sun, Jan 30, 2022 at 12:03 PM Ethan Smith 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

On Sun, Jan 30, 2022 at 7:37 PM Irit Katriel 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

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 wrote:

...

On Sat, Jan 29, 2022 at 7:38 PM Inada Naoki wrote:

...

On Sun, Jan 30, 2022 at 12:03 PM Ethan Smith 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

_______________________________________________ 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, Jan 30, 2022 at 8:42 AM Mats Wichmann wrote:

On 1/30/22 04:45, Inada Naoki wrote:

...

On Sun, Jan 30, 2022 at 7:37 PM Irit Katriel 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.

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).

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 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 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 wrote:

...

On Sun, Jan 30, 2022 at 8:42 AM Mats Wichmann 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-c...

-- --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-c...

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 Mon, 31 Jan 2022 at 23:29, Greg Ewing 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 님이 작성:

...

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/

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 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/

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, 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