Replies: 8 comments 5 replies
-
|
As addition: PR #1139 has a request for additional review but the merge itself is allowed. Which makes it unclear , to me, how to handle such a PR. What if reviews of specific persons are not submitted (the large #1078 is/was a nice example). Which may be a different topic: what is expected from a review itself? |
Beta Was this translation helpful? Give feedback.
-
Just because it's not required by a technical constraint, it doesn't mean we shouldn't do it. I think there is a general agreement to do this anyway, but we can still discuss it of course. I personally think it's nice to be allowed to merge very simple fixes (like fixing typos in texts) without a review, so I wouldn't change that. |
Beta Was this translation helpful? Give feedback.
-
|
I'm pulling your discussion from #1157 here:
My proposal for this: Only for very simple PRs (such as fixing typos for example) should we merge without approval.
Yes, this is a point to discuss in the next meetup, but as the time in those meetups is always very limited, we can probably propose some ground rules in this discussion...? |
Beta Was this translation helpful? Give feedback.
-
|
I agree that when a PR is approved and it passed the work flow checks
When a PR can not be merged, for what ever reason, there are two possible routes, I feel:
In general I am always in favor of having a reviewer, even for the tiny changes, just to make sure no silly problem is going to be injected but it stands to reason that anything in As for |
Beta Was this translation helpful? Give feedback.
-
|
I can agree with both of you! Maybe we can use this (outdated) wiki page https://github.com/foodcoops/foodsoft/wiki/Merging-pull-requests to persist a agreement? |
Beta Was this translation helpful? Give feedback.
-
|
I'd propose to change the Guideline like the following. If I forgot someone in the maintainers role or anyone doesn't feels like beeing in it, just let me know. MaintainershipMaintainers of the foodcoops/foodsoft repository are responsible for handling contributions. When a pull request is submitted, a maintainer reviews the changes (see [[Developing Guidelines]]). At this time (2025-05-22) the following person have the maintainer role:
How to Pull-Request
PluginsWhen merging a pull request for a plugin, there are some other things to check and do:
|
Beta Was this translation helpful? Give feedback.
-
|
I have an additional case to consider: there are several (very) old PR's. See also #1135 I fear that waiting to discuss all of them will mean they get never resolved so I want to propose a partial solution for any PR older than 6 months:
As I mentioned earlier: I am a big proponent for having a manageable list of PR's which we actually intent to merge and cut all the stale and useless PR's. Revisiting those, without a clear resolution, will keep costing time. And to the current state: there are a few which I believe can be closed without any problem. I am happy to do that and be hit on the head by any of you if I close one that is needed. At least it becomes clear that way 😇 Another option is to close all (older 6 months) PR's which are not added by any of the current group as there is no advocate for them anyway. Thoughts?? |
Beta Was this translation helpful? Give feedback.
-
|
thanks for your additions! I've tried to integrate them into the wiki article. feel free to adapt as you like! |
Beta Was this translation helpful? Give feedback.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
-
Hi folks,
I noticed that there is no required review step for
contributers/membersof the repo.Personally I am not a big fan of
self-merging. I noticed I am now allowed to merge #1080 (for instance, which is by itself rather harmless assuming the translations are not weird 😇 ) to the application. But in general I prefer that a PR is always reviewed, at least glanced over, by someone else?I take it the passing of the checks suffices at this point and I can imagine that a required review may block too much but I am not really comfortable with just merging my own work.
What are the general considerations here?
Beta Was this translation helpful? Give feedback.
All reactions