Thread: [PATCH] CF app: add "Returned: Needs more interest"
[Trying -hackers rather than -www this time, since the impacted users are here.] There are occasionally patches that are dutifully rebased by a responsive author, all feedback implemented... but there have been no reviews for a while, and there's no sign of any on the way. This case seems to tie us up in knots. We don't want to Reject since the patch is fine, it's just not a current priority; and we don't want to Return with Feedback because there is no feedback to act upon. Since no one wants to close it, they can drag on forever, with hopeful authors rebasing eternally. I ended up closing several patches like this with RwF, but I felt the need to write a huge explanation in the accompanying email. This has been discussed before (e.g. [1]); the two competing proposals I've seen are to add a new close state or to simply remove the "with Feedback" and treat all Returned patches the same. I can see the case for minimizing the number of choices, but in this patchset, I've opted to implement a new state. I think it's useful to communicate to a contributor that their next steps, if they still want to pursue them, need to be focused on coalition building rather than code changes. And I think Returned with Feedback has a useful meaning already. 0001 just adds the "Returned: Needs more interest" state to the database and makes it available to the UI. The (optional) 0002 goes farther, and puts the two Returned states along with the Next CF state into a new "Deferred" section in the UI. (It's UI-only; the app still treats them as closed patches for the purposes of the CF.) This emphasizes the distinction between Returned and Rejected: Rejected is meant to be a full stop to the story, while Returned is more of a comma. Screenshots attached. WDYT? --Jacob [1] https://www.postgresql.org/message-id/flat/3905363.1633288498%40sss.pgh.pa.us
Attachment
Hi, On Tue, Aug 02, 2022 at 03:55:28PM -0700, Jacob Champion wrote: > [Trying -hackers rather than -www this time, since the impacted users are here.] > > There are occasionally patches that are dutifully rebased by a > responsive author, all feedback implemented... but there have been no > reviews for a while, and there's no sign of any on the way. This case > seems to tie us up in knots. We don't want to Reject since the patch > is fine, it's just not a current priority; and we don't want to Return > with Feedback because there is no feedback to act upon. Since no one > wants to close it, they can drag on forever, with hopeful authors > rebasing eternally. I ended up closing several patches like this with > RwF, but I felt the need to write a huge explanation in the > accompanying email. > [...] > [1] https://www.postgresql.org/message-id/flat/3905363.1633288498%40sss.pgh.pa.us I'm personally fine with the current statutes, as closing a patch with RwF explaining that there was no interest is still a feedback, and having a different status won't make it any more pleasant for both the CFM and the author. My biggest complaint here is that it doesn't really do anything to try to improve the current situation (lack of review and/or lack of committer interest). Maybe it would be better to discuss some clear rules and thresholds on when action should be taken on such patches. It doesn't have to be closing the CF entry directly but instead sending some email to ask for community / committer feedback as in the thread you pointed, and document that in the commitfest wiki page.
On Tue, Aug 2, 2022 at 8:00 PM Julien Rouhaud <rjuju123@gmail.com> wrote: > I'm personally fine with the current statutes, as closing a patch with RwF > explaining that there was no interest is still a feedback, Hi Julien, Making that explanation each time we intend to close a patch "needs interest" takes a lot of time and wordsmithing. "Returned with feedback" clearly has an established meaning to the community, and this is counter to that meaning, so people just avoid using it that way. When they do, miscommunications happen easily, which can lead to authors reopening patches thinking that there's been some kind of mistake (as happened to at least one of the patches in this past CF, which I had to close again). Language and cultural differences likely exacerbate the problem, so the less ad hoc messaging a CFM has to do to explain that "this is RwF but not actually RwF", the better. > and having a > different status won't make it any more pleasant for both the CFM and the > author. "More pleasant" is not really the goal here. I don't think it should ever be pleasant for a CFM to return someone's patch when it hasn't received review, and it's certainly not going to be pleasant for the author. But we can be more honest and clear about why we're returning it, and hopefully make it less unpleasant. > My biggest complaint here is that it doesn't really do anything to try to > improve the current situation (lack of review and/or lack of committer > interest). It's not really meant to improve that. This is just trying to move the needle a little bit, in a way that's been requested several times. > Maybe it would be better to discuss some clear rules and thresholds on when > action should be taken on such patches. I think that's also important to discuss, and I have thoughts on that too, but I don't think the discussions for these sorts of incremental changes should wait for that discussion. --Jacob
Hi, On Wed, Aug 03, 2022 at 08:58:49AM -0700, Jacob Champion wrote: > On Tue, Aug 2, 2022 at 8:00 PM Julien Rouhaud <rjuju123@gmail.com> wrote: > > I'm personally fine with the current statutes, as closing a patch with RwF > > explaining that there was no interest is still a feedback, > > Making that explanation each time we intend to close a patch "needs > interest" takes a lot of time and wordsmithing. "Returned with > feedback" clearly has an established meaning to the community, and > this is counter to that meaning, so people just avoid using it that > way. > > When they do, miscommunications happen easily, which can lead to > authors reopening patches thinking that there's been some kind of > mistake (as happened to at least one of the patches in this past CF, > which I had to close again). Language and cultural differences likely > exacerbate the problem, so the less ad hoc messaging a CFM has to do > to explain that "this is RwF but not actually RwF", the better. > > > and having a > > different status won't make it any more pleasant for both the CFM and the > > author. > > "More pleasant" is not really the goal here. I don't think it should > ever be pleasant for a CFM to return someone's patch when it hasn't > received review, and it's certainly not going to be pleasant for the > author. But we can be more honest and clear about why we're returning > it, and hopefully make it less unpleasant. > > > My biggest complaint here is that it doesn't really do anything to try to > > improve the current situation (lack of review and/or lack of committer > > interest). > > It's not really meant to improve that. This is just trying to move the > needle a little bit, in a way that's been requested several times. > > > Maybe it would be better to discuss some clear rules and thresholds on when > > action should be taken on such patches. > > I think that's also important to discuss, and I have thoughts on that > too, but I don't think the discussions for these sorts of incremental > changes should wait for that discussion. First of all, I didn't want to imply that rejecting a patch should be pleasant, sorry if that sounded that way. It's not that I'm opposed to adding that status, I just don't see how it's really going to improve the situation on its own. Or maybe because it wouldn't make any difference to me as a patch author to get my patches returned "with feedback" or "for any other reason" if they are ignored. I'm afraid that patches will still be left alone to rot and there still be no clear rules on what to do and when, reminder for CFM and such, and that this new status would never be used anyway. So I guess I will just stop hijacking this thread and wait for a discussion on that, sorry for the noise.
On Wed, Aug 3, 2022 at 10:09 AM Julien Rouhaud <rjuju123@gmail.com> wrote: > First of all, I didn't want to imply that rejecting a patch should be pleasant, > sorry if that sounded that way. No worries, I don't think it really sounded that way. :D > It's not that I'm opposed to adding that status, I just don't see how it's > really going to improve the situation on its own. If the situation you're referring to is the fact that we have a lot of patches sitting without review, it won't improve that situation, I agree. The situation I'm looking at, though, is where we have a dozen patches floating forward that multiple CFMs in a row feel should be returned, but they won't because claiming "they have feedback" is clearly unfair to the author. I think this will improve that situation. > Or maybe because it wouldn't > make any difference to me as a patch author to get my patches returned "with > feedback" or "for any other reason" if they are ignored. Sure. I think this change helps the newer contributors (and the CFMs talking to them) more than it helps the established ones. I'm in your boat, where I don't personally care how a patch of mine is returned (and I'm fine with Withdrawing them myself). But I'm also paid to do this. From some of my past experiences with other projects, I tend to feel more sensitive to bad communication if I've developed a patch using volunteer hours, on evenings or weekends. > I'm afraid that > patches will still be left alone to rot and there still be no clear rules on > what to do and when, reminder for CFM and such, and that this new status would > never be used anyway. So I guess I will just stop hijacking this thread and > wait for a discussion on that, sorry for the noise. Well, here, let's keep that conversation going too while there's momentum. One sec while I switch Subjects and continue... --Jacob
[was: CF app: add "Returned: Needs more interest"] On Wed, Aug 3, 2022 at 10:09 AM Julien Rouhaud <rjuju123@gmail.com> wrote: > I'm afraid that > patches will still be left alone to rot and there still be no clear rules on > what to do and when, reminder for CFM and such, and that this new status would > never be used anyway. Yeah, so the lack of clear rules is an issue -- maybe not because we can't work without them (we have, clearly, and we can continue to do so) but because each of us kind of makes it up as we go along? When discussions about these "rules" happen on the list, it doesn't always happen with the same people, and opinions can vary wildly. There have been a couple of suggestions recently: - Revamp the CF Checklist on the wiki. I plan to do so later this month, but that will still need some community review. - Provide in-app explanations and documentation for some of the less obvious points. (What should the target version be? What's the difference between Rejected and Returned?) Is that enough, or should we do more? My preference, as I think Daniel also said in a recent thread, would be for most of this information to be in the application somewhere. That would make it more immediately accessible to everyone. (The tradeoff is, it gets harder to update.) --Jacob
Hi, On 2022-08-03 10:52:36 -0700, Jacob Champion wrote: > The situation I'm looking at, though, is where we have a dozen patches > floating forward that multiple CFMs in a row feel should be returned, > but they won't because claiming "they have feedback" is clearly unfair > to the author. I think this will improve that situation. What patches are we concretely talking about? My impression is that a lot of the patches floating from CF to CF have gotten sceptical feedback and at best a minor amount of work to address that has been done. Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes:
> My impression is that a lot of the patches floating from CF to CF have gotten
> sceptical feedback and at best a minor amount of work to address that has been
> done.
That I think is a distinct issue: nobody wants to take on the
unpleasant job of saying "no, we don't want this" in a final way.
We may raise some objections but actually rejecting a patch is hard.
So it tends to slide forward until the author gives up.
regards, tom lane
On 8/3/22 11:41, Andres Freund wrote: > What patches are we concretely talking about?> > My impression is that a lot of the patches floating from CF to CF have gotten > sceptical feedback and at best a minor amount of work to address that has been > done. - https://commitfest.postgresql.org/38/2482/ - https://commitfest.postgresql.org/38/3338/ - https://commitfest.postgresql.org/38/3181/ - https://commitfest.postgresql.org/38/2918/ - https://commitfest.postgresql.org/38/2710/ - https://commitfest.postgresql.org/38/2266/ (this one was particularly miscommunicated during the first RwF) - https://commitfest.postgresql.org/38/2218/ - https://commitfest.postgresql.org/38/3256/ - https://commitfest.postgresql.org/38/3310/ - https://commitfest.postgresql.org/38/3050/ Looking though, some of those have received skeptical feedback as you say, but certainly not all; not even a majority IMO. (Even if they'd all received skeptical feedback, if the author replies in good faith and is met with silence for months, we need to not keep stringing them along.) --Jacob
Hi, On 2022-08-03 12:06:03 -0700, Jacob Champion wrote: > On 8/3/22 11:41, Andres Freund wrote: > > What patches are we concretely talking about?> > > My impression is that a lot of the patches floating from CF to CF have gotten > > sceptical feedback and at best a minor amount of work to address that has been > > done. > > - https://commitfest.postgresql.org/38/2482/ Hm - "Returned: Needs more interest" doesn't seem like it'd have been more descriptive? It was split off a patchset that was committed at the tail end of 15 (and which still has *severe* code quality issues). Imo having a CF entry before the rest of the jsonpath stuff made it in doesn't seem like a good idea. > - https://commitfest.postgresql.org/38/3338/ Here it'd have fit. > - https://commitfest.postgresql.org/38/3181/ FWIW, I mentioned at least once that I didn't think this was worth pursuing. > - https://commitfest.postgresql.org/38/2918/ Hm, certainly not a lot of review activity. > - https://commitfest.postgresql.org/38/2710/ A good bit of this was committed in some form with a decent amount of review activity for a while. > - https://commitfest.postgresql.org/38/2266/ (this one was particularly > miscommunicated during the first RwF) I'd say misunderstanding than miscommunication... It seems partially stalled due to the potential better approach based on https://www.postgresql.org/message-id/flat/15848.1576515643%40sss.pgh.pa.us ? In which case RwF doesn't seem to inappropriate. > - https://commitfest.postgresql.org/38/2218/ Yep. > - https://commitfest.postgresql.org/38/3256/ Yep. > - https://commitfest.postgresql.org/38/3310/ I don't really understand why this has been RwF'd, doesn't seem that long since the last review leading to changes. > - https://commitfest.postgresql.org/38/3050/ Given that a non-author did a revision of the patch, listed a number of TODO items and said "I'll create regression tests firstly." - I don't think "lacks interest" would have been appropriate, and RwF is? > (Even if they'd all received skeptical feedback, if the author replies in > good faith and is met with silence for months, we need to not keep stringing > them along.) I agree very much with that - just am doubtful that "lacks interest" is a good way of dealing with it, unless we just want to treat it as a nicer sounding "rejected". Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes:
> I agree very much with that - just am doubtful that "lacks interest" is a good
> way of dealing with it, unless we just want to treat it as a nicer sounding
> "rejected".
I think there is a difference. "Lacks interest" suggests that there
is a path forward for the patch, namely (as Jacob has mentioned
repeatedly) doing some sort of consensus-building that it's worth
pursuing. The author may or may not have the interest/skills to do
that, but it's possible that it could happen. "Rejected" says "don't
bother pursuing this, it's a bad idea". Neither of these seems the
same as RWF, which I think we mostly understand to mean "this patch
has technical problems that can probably be fixed".
regards, tom lane
On Wed, 3 Aug 2022 at 20:04, Jacob Champion <jchampion@timescale.com> wrote: > > [was: CF app: add "Returned: Needs more interest"] > > On Wed, Aug 3, 2022 at 10:09 AM Julien Rouhaud <rjuju123@gmail.com> wrote: > > I'm afraid that > > patches will still be left alone to rot and there still be no clear rules on > > what to do and when, reminder for CFM and such, and that this new status would > > never be used anyway. > > Yeah, so the lack of clear rules is an issue -- maybe not because we > can't work without them (we have, clearly, and we can continue to do > so) but because each of us kind of makes it up as we go along? When > discussions about these "rules" happen on the list, it doesn't always > happen with the same people, and opinions can vary wildly. > > There have been a couple of suggestions recently: > - Revamp the CF Checklist on the wiki. I plan to do so later this > month, but that will still need some community review. > - Provide in-app explanations and documentation for some of the less > obvious points. (What should the target version be? What's the > difference between Rejected and Returned?) > > Is that enough, or should we do more? "The CF Checklist" seems to refer to only the page that is (or seems to be) intended for the CFM only. We should probably also update the pages of "Commitfest", "Submitting a patch", "Reviewing a Patch", "So, you want to be a developer?", and the "Developer FAQ" page, which doesn't have to be more than removing outdated information and refering to any (new) documentation on how to participate in the PostgreSQL development and/or Commitfest workflow as a non-CFM. Additionally, we might want to add extra text to the "developers" section of the main website [0] to refer to any new documentation. This suggestion does depend on whether the new documentation has a high value for potential community members. Lastly, a top-level CONTRIBUTING.md file in git repositories is also often used as an entry point for potential contributors. I don't suggest we copy all documentation into the main repo, just that a pointer to our existing contributer entry documentation in such a file could help lower the barrier of entry. As an example, the GitHub mirror of the main PostgreSQL repository receives a decent amount of pull request traffic. When a project has a CONTRIBUTING.md -file at the top level people writing the pull request message will be pointed to those contributing guidelines. This could Thank you for raising this to a topical thread. Kind regards, Matthias van de Meent [0] https://www.postgresql.org/developer/
Hi Andres, My intention had not quite been for this to be a referendum on the decision for every patch -- we can do that if it helps, but I don't think we necessarily have to have unanimity on the bucketing for every patch in order for the new state to be useful. On 8/3/22 12:46, Andres Freund wrote: >> - https://commitfest.postgresql.org/38/2482/ > > Hm - "Returned: Needs more interest" doesn't seem like it'd have been more > descriptive? It was split off a patchset that was committed at the tail end of > 15 (and which still has *severe* code quality issues). Imo having a CF entry > before the rest of the jsonpath stuff made it in doesn't seem like a good > idea There were no comments about code quality issues on the thread that I can see, and there were three people who independently said "I don't know why this isn't getting review." Seems like a shoe-in for "needs more interest". >> - https://commitfest.postgresql.org/38/3338/ > > Here it'd have fit. Okay. That's one. >> - https://commitfest.postgresql.org/38/3181/ > > FWIW, I mentioned at least once that I didn't think this was worth pursuing. (I don't see that comment on that thread? You mentioned it needed a rebase.) IMO, mentioning that something is not worth pursuing is not actionable feedback. It's a declaration of non-interest in the mildest case, and a Rejection in the strongest case. But let's please not say "meh" and then Return with Feedback; an author can't do anything with that. >> - https://commitfest.postgresql.org/38/2918/ > > Hm, certainly not a lot of review activity. That's two. >> - https://commitfest.postgresql.org/38/2710/ > > A good bit of this was committed in some form with a decent amount of review > activity for a while. But then the rest of it stalled. Something has to be done with the open entry. >> - https://commitfest.postgresql.org/38/2266/ (this one was particularly >> miscommunicated during the first RwF) > > I'd say misunderstanding than miscommunication... The CFM sending it said, "It seems there has been no activity since last version of the patch so I don't think RwF is correct" [1], and then the email sent said "you are encouraged to send a new patch [...] with the suggested changes." But there were no suggested changes left to make. This really highlights, for me, why the two states should not be combined into one. > It seems partially stalled due to the potential better approach based on > https://www.postgresql.org/message-id/flat/15848.1576515643%40sss.pgh.pa.us ? > In which case RwF doesn't seem to inappropriate. Those comments are, as far as I can tell, not in the thread. (And the new thread you linked is also stalled.) >> - https://commitfest.postgresql.org/38/2218/ > > Yep. That's three. >> - https://commitfest.postgresql.org/38/3256/ > > Yep. That's four. >> - https://commitfest.postgresql.org/38/3310/ > > I don't really understand why this has been RwF'd, doesn't seem that long > since the last review leading to changes. Eight months without feedback, when we expect authors to turn around a patch in two weeks or less to avoid being RwF'd, is a long time IMHO. I don't think a patch should sit motionless in CF for eight months; it's not at all fair to the author. >> - https://commitfest.postgresql.org/38/3050/ > > Given that a non-author did a revision of the patch, listed a number of TODO > items and said "I'll create regression tests firstly." - I don't think "lacks > interest" would have been appropriate, and RwF is? That was six months ago, and prior to that there was another six month silence. I'd say that lacks interest, and I don't feel like it's currently reviewable in CF. >> (Even if they'd all received skeptical feedback, if the author replies in >> good faith and is met with silence for months, we need to not keep stringing >> them along.) > > I agree very much with that - just am doubtful that "lacks interest" is a good > way of dealing with it, unless we just want to treat it as a nicer sounding > "rejected". Tom summed up my position well: there's a difference between those two that is both meaningful and actionable for contributors. Is there an alternative you'd prefer? Thanks for the discussion! --Jacob [1] https://www.postgresql.org/message-id/20211004071249.GA6304%40ahch-to
On Wed, Aug 3, 2022 at 2:05 PM Matthias van de Meent <boekewurm+postgres@gmail.com> wrote: > On Wed, 3 Aug 2022 at 20:04, Jacob Champion <jchampion@timescale.com> wrote: > > Is that enough, or should we do more? > > "The CF Checklist" seems to refer to only the page that is (or seems > to be) intended for the CFM only. We should probably also update the > pages of "Commitfest", "Submitting a patch", "Reviewing a Patch", "So, > you want to be a developer?", and the "Developer FAQ" page, which > doesn't have to be more than removing outdated information and > refering to any (new) documentation on how to participate in the > PostgreSQL development and/or Commitfest workflow as a non-CFM. Agreed, a sweep of those materials would be helpful as well. I'm personally focused on CFM tasks, since it's fresh in my brain and documentation is almost non-existent for it, but if you have ideas for those areas, I definitely don't want to shut down that line of the conversation. > Additionally, we might want to add extra text to the "developers" > section of the main website [0] to refer to any new documentation. > This suggestion does depend on whether the new documentation has a > high value for potential community members. Right. So what kinds of info do we want to highlight in this documentation, to make it high-quality? Drawing from some of the questions I've seen recently, we could talk about - CF "power" structure (perhaps simply to highlight that the CFM has no additional authority to get patches in) - the back-and-forth process on the mailing list, maybe including expected response times - what to do when a patch is returned (or rejected) > As an example, the GitHub mirror of the main PostgreSQL repository > receives a decent amount of pull request traffic. When a project has a > CONTRIBUTING.md -file at the top level people writing the pull request > message will be pointed to those contributing guidelines. This could (I think some text got cut here.) The mirror bot will point you to the "So, you want to be a developer?" wiki when you open a PR, but I agree that a CONTRIBUTING doc would help prevent that small embarrassment. --Jacob
Hi, On 2022-08-04 11:19:28 -0700, Jacob Champion wrote: > My intention had not quite been for this to be a referendum on the > decision for every patch -- we can do that if it helps, but I don't > think we necessarily have to have unanimity on the bucketing for every > patch in order for the new state to be useful. Sorry, I should have been clearer. It wasn't mine either! I was just trying to understand what you see as the usecase / get a better feel for it. I'm now a bit more convinced it's useful than before. > >> - https://commitfest.postgresql.org/38/3310/ > > > > I don't really understand why this has been RwF'd, doesn't seem that long > > since the last review leading to changes. > > Eight months without feedback, when we expect authors to turn around a > patch in two weeks or less to avoid being RwF'd, is a long time IMHO. Why is it better to mark it as lacks interested than RwF if there actually *has* been feedback? > I don't think a patch should sit motionless in CF for eight months; it's not > at all fair to the author. It's not great, I agree, but wishes don't conjure up resources :( > >> - https://commitfest.postgresql.org/38/3050/ > > > > Given that a non-author did a revision of the patch, listed a number of TODO > > items and said "I'll create regression tests firstly." - I don't think "lacks > > interest" would have been appropriate, and RwF is? > > That was six months ago, and prior to that there was another six month > silence. I'd say that lacks interest, and I don't feel like it's > currently reviewable in CF. I don't think the entry needs more review - it needs changes: https://www.postgresql.org/message-id/CAOKkKFtc45uNFoWYOCo4St19ayxrh-_%2B4TnZtwxGZz6-3k_GSA%40mail.gmail.com That contains quite a few things that should be changed. A patch that has gotten feedback, but that feedback hasn't been processed pretty much is the definition of RwF, no? > >> (Even if they'd all received skeptical feedback, if the author replies in > >> good faith and is met with silence for months, we need to not keep stringing > >> them along.) > > > > I agree very much with that - just am doubtful that "lacks interest" is a good > > way of dealing with it, unless we just want to treat it as a nicer sounding > > "rejected". > > Tom summed up my position well: there's a difference between those two > that is both meaningful and actionable for contributors. Is there an > alternative you'd prefer? I agree that "lacks interest" could be useful. But I'm wary of it becoming just a renaming if we end up marking patches that should be RwF or rejected as "lacks interest". Greetings, Andres Freund
On Thu, 4 Aug 2022 at 20:38, Jacob Champion <jchampion@timescale.com> wrote: > > On Wed, Aug 3, 2022 at 2:05 PM Matthias van de Meent > <boekewurm+postgres@gmail.com> wrote: > > On Wed, 3 Aug 2022 at 20:04, Jacob Champion <jchampion@timescale.com> wrote: > > > Is that enough, or should we do more? > > > > "The CF Checklist" seems to refer to only the page that is (or seems > > to be) intended for the CFM only. We should probably also update the > > pages of "Commitfest", "Submitting a patch", "Reviewing a Patch", "So, > > you want to be a developer?", and the "Developer FAQ" page, which > > doesn't have to be more than removing outdated information and > > refering to any (new) documentation on how to participate in the > > PostgreSQL development and/or Commitfest workflow as a non-CFM. > > Agreed, a sweep of those materials would be helpful as well. I'm > personally focused on CFM tasks, since it's fresh in my brain and > documentation is almost non-existent for it, but if you have ideas for > those areas, I definitely don't want to shut down that line of the > conversation. Nor would I want to hold you back on CFM documentation. > > Additionally, we might want to add extra text to the "developers" > > section of the main website [0] to refer to any new documentation. > > This suggestion does depend on whether the new documentation has a > > high value for potential community members. > > Right. So what kinds of info do we want to highlight in this > documentation, to make it high-quality? I think it would be a combined and abbreviated version of the detailed manuals that we (will) have: The pages "Submitting a patch" and "Reviewing a patch" on the wiki, and the CommitFest manual (plus potentially info on CFBot). The first part of "So, you want to be a developer?" seems like a very good starting point for dense, high-quality entry-level documentation. Each section should then further refer to the relevant sections of the "Developer FAQ" and the "Submitting / Reviewing a Patch" pages for the in-and-outs of the specific procedure (such as "installing development dependencies", "reviewing changes", "code style", etc.). > Drawing from some of the questions I've seen recently, we could talk about > - CF "power" structure (perhaps simply to highlight that the CFM has > no additional authority to get patches in) > - the back-and-forth process on the mailing list, maybe including > expected response times > - what to do when a patch is returned (or rejected) > > > As an example, the GitHub mirror of the main PostgreSQL repository > > receives a decent amount of pull request traffic. When a project has a > > CONTRIBUTING.md -file at the top level people writing the pull request > > message will be pointed to those contributing guidelines. This could > > (I think some text got cut here.) ... This could help reduce the amount of mis-addressed (maybe better word: mis-located?) contributions, and potentially help the contributor get involved at -hackers. Indeed this process is much more involved than 'just' opening a pull request, but at least it is now slightly more visible. > The mirror bot will point you to the "So, you want to be a developer?" > wiki when you open a PR, but I agree that a CONTRIBUTING doc would > help prevent that small embarrassment. That's news to me, but nice to see some improvements there. I have previously noticed that there were PRs on GitHub that went unnoticed for several weeks, so this bot is a significant improvement. Kind regards, Matthias van de Meent
On Thu, Aug 4, 2022 at 3:00 PM Andres Freund <andres@anarazel.de> wrote: > On 2022-08-04 11:19:28 -0700, Jacob Champion wrote: > > My intention had not quite been for this to be a referendum on the > > decision for every patch -- we can do that if it helps, but I don't > > think we necessarily have to have unanimity on the bucketing for every > > patch in order for the new state to be useful. > > Sorry, I should have been clearer. It wasn't mine either! I was just trying to > understand what you see as the usecase / get a better feel for it. I'm now a > bit more convinced it's useful than before. Great! > > >> - https://commitfest.postgresql.org/38/3310/ > > > > > > I don't really understand why this has been RwF'd, doesn't seem that long > > > since the last review leading to changes. > > > > Eight months without feedback, when we expect authors to turn around a > > patch in two weeks or less to avoid being RwF'd, is a long time IMHO. > > Why is it better to mark it as lacks interested than RwF if there actually > *has* been feedback? Because I don't think the utility of RwF is in saying "we gave you feedback once and then ghosted"; I think it's in saying "this patchset needs work before the next round of review." If an author has responded to the feedback and the patchset is just sitting there for months, the existence of the feedback is less relevant. > > I don't think a patch should sit motionless in CF for eight months; it's not > > at all fair to the author. > > It's not great, I agree, but wishes don't conjure up resources :( I see this less as a wish for resources, and more as an honest admission -- we don't currently have enough resources to give each patch the eyes it deserves, so if an author finds themselves in this state, they'll have to put in some more work to find those eyes somewhere. > > >> - https://commitfest.postgresql.org/38/3050/ > > > > > > Given that a non-author did a revision of the patch, listed a number of TODO > > > items and said "I'll create regression tests firstly." - I don't think "lacks > > > interest" would have been appropriate, and RwF is? > > > > That was six months ago, and prior to that there was another six month > > silence. I'd say that lacks interest, and I don't feel like it's > > currently reviewable in CF. > > I don't think the entry needs more review - it needs changes: > https://www.postgresql.org/message-id/CAOKkKFtc45uNFoWYOCo4St19ayxrh-_%2B4TnZtwxGZz6-3k_GSA%40mail.gmail.com > That contains quite a few things that should be changed. > > A patch that has gotten feedback, but that feedback hasn't been processed > pretty much is the definition of RwF, no? Looking through again, I see now what you're saying. Yes, I agree that RwF would have been a fine fit there. > I agree that "lacks interest" could be useful. But I'm wary of it becoming > just a renaming if we end up marking patches that should be RwF or rejected as > "lacks interest". Agreed. This probably bleeds over into the other documentation thread a bit -- how do we want to communicate the subtle points to people in a CF? --Jacob
Hi, On 2022-08-08 08:37:41 -0700, Jacob Champion wrote: > Agreed. This probably bleeds over into the other documentation thread > a bit -- how do we want to communicate the subtle points to people in > a CF? We should write a docs patch for it and then reference if from a bunch of places. I started down that road a few years back [1] but unfortunately lost steam. Regards, Andres [1] https://postgr.es/m/20180302224056.3fps7kc6hokqk3th%40alap3.anarazel.de
On Wed, Aug 3, 2022 at 02:53:23PM -0400, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > My impression is that a lot of the patches floating from CF to CF have gotten > > sceptical feedback and at best a minor amount of work to address that has been > > done. > > That I think is a distinct issue: nobody wants to take on the > unpleasant job of saying "no, we don't want this" in a final way. > We may raise some objections but actually rejecting a patch is hard. > So it tends to slide forward until the author gives up. Agreed. There is a sense when I look at patches in that status that they seem like a good idea to someone and could be useful to someone, but the overhead or complexity it would add to the software doesn't seem warranted. It is complex to explain that to someone, and since it is a judgement call, not worth the argument. -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com Indecision is a decision. Inaction is an action. Mark Batterson
On Mon, Aug 8, 2022 at 8:45 AM Andres Freund <andres@anarazel.de> wrote: > On 2022-08-08 08:37:41 -0700, Jacob Champion wrote: > > Agreed. This probably bleeds over into the other documentation thread > > a bit -- how do we want to communicate the subtle points to people in > > a CF? > > We should write a docs patch for it and then reference if from a bunch of > places. I started down that road a few years back [1] but unfortunately lost > steam. As we approach a new CF, I'm reminded of this patch again. Are there any concerns preventing a consensus here, that I can help with? I can draft the docs patch that Andres has suggested, if that's seen as a prerequisite. Thanks, --Jacob
On Wed, 26 Oct 2022 at 04:25, Jacob Champion <jchampion@timescale.com> wrote: > > On Mon, Aug 8, 2022 at 8:45 AM Andres Freund <andres@anarazel.de> wrote: > > On 2022-08-08 08:37:41 -0700, Jacob Champion wrote: > > > Agreed. This probably bleeds over into the other documentation thread > > > a bit -- how do we want to communicate the subtle points to people in > > > a CF? > > > > We should write a docs patch for it and then reference if from a bunch of > > places. I started down that road a few years back [1] but unfortunately lost > > steam. > > As we approach a new CF, I'm reminded of this patch again. > > Are there any concerns preventing a consensus here, that I can help > with? I can draft the docs patch that Andres has suggested, if that's > seen as a prerequisite. The patch does not apply on top of HEAD as in [1], please post a rebased patch: === Applying patches on top of PostgreSQL commit ID e351f85418313e97c203c73181757a007dfda6d0 === === applying patch ./0001-Add-a-Returned-Needs-more-interest-close-code.patch patching file pgcommitfest/commitfest/migrations/0006_alter_patchoncommitfest_status.py can't find file to patch at input line 57 Perhaps you used the wrong -p or --strip option? The text leading up to this was: -------------------------- |diff --git a/pgcommitfest/commitfest/models.py b/pgcommitfest/commitfest/models.py |index 28722f0..433eb4a 100644 |--- a/pgcommitfest/commitfest/models.py |+++ b/pgcommitfest/commitfest/models.py -------------------------- No file to patch. Skipping patch. 3 out of 3 hunks ignored can't find file to patch at input line 85 [1] - http://cfbot.cputube.org/patch_41_3991.log Regards, Vignesh
On Tue, Jan 3, 2023 at 4:14 AM vignesh C <vignesh21@gmail.com> wrote: > The patch does not apply on top of HEAD as in [1], please post a rebased patch: Hi Vignesh -- this is a patch for the CF app, not the Postgres repo, so cfbot won't be able to apply it. Let me know if there's a better place for me to put it. Thanks, --Jacob
On Tue, 3 Jan 2023 at 22:01, Jacob Champion <jchampion@timescale.com> wrote: > > On Tue, Jan 3, 2023 at 4:14 AM vignesh C <vignesh21@gmail.com> wrote: > > The patch does not apply on top of HEAD as in [1], please post a rebased patch: > > Hi Vignesh -- this is a patch for the CF app, not the Postgres repo, > so cfbot won't be able to apply it. Let me know if there's a better > place for me to put it. I'm not sure if this should be included in commitfest as we generally include the postgres repository patches in the commitfest. I felt we could have the discussion in the thread and remove the entry from commitfest. Regards, Vignesh
On Tue, Jan 3, 2023 at 8:56 PM vignesh C <vignesh21@gmail.com> wrote: > I'm not sure if this should be included in commitfest as we generally > include the postgres repository patches in the commitfest. I felt we > could have the discussion in the thread and remove the entry from > commitfest. Is there a good way to remind people that, hey, this exists as a patchset? (Other than me pinging the list every so often.) --Jacob
On Wed, Jan 4, 2023 at 9:33 AM Jacob Champion <jchampion@timescale.com> wrote: > Is there a good way to remind people that, hey, this exists as a > patchset? (Other than me pinging the list every so often.) I've withdrawn this patchset for now, but if anyone has any ideas on where and how I can better propose features for CF itself, I'm all ears. Thanks, --Jacob