Merge procedure?

[RayOei]

Hi folks,

I noticed that there is no required review step for contributers/members of 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?

[RayOei]

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).
The code itself seems fine to me by face value. But I haven't checked the functioning (yet) and I don't know if this functionality is already covered by tests, for instance.

Which may be a different topic: what is expected from a review itself?
Are there specific things to look out for?
Should we strive for adding tests with new code, therefore improving the (poor as far as I can make out) coverage?
Other?

[lentschi]

I noticed that there is no required review step for contributers/members of the repo.

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.

[lentschi]

I'm pulling your discussion from #1157 here:

@robwa

@yksflip Thank you for your review. May I merge this or should we wait for further reviews? Who decides when to merge and who does it?

My proposal for this:
One approving review from another active developer should suffice for reasonable-sized changes. The merge should subsequently be performed by the person creating the PR (unless a third party was involved in coding, in which case it would be fine for them to merge too).

Only for very simple PRs (such as fixing typos for example) should we merge without approval.

@yksflip

that's a very good question, robwa raised this question here already: #1139
we should come up with some procedure soo 😅

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

[RayOei]

I agree that when a PR is approved and it passed the work flow checks

1. it can be merged
2. it should be merged as there is no reason to leave it dangling
3. merged by creator or maintainer within a reasonable time frame

When a PR can not be merged, for what ever reason, there are two possible routes, I feel:

1. raise objections and adapt to make it acceptable
2. reject and remove
   a) as it can or will not (never?) be merged in that specific form
   b) as it adds confusion further down the road if long living PR's are not resolved (which ties a bit into Cleanup issues #1135).

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 translations and readme (and similar) may be exempt based on everybody's good judgement the change is not contentious.

As for reasonable-sized changes: I am a big proponent of having many small specific (feature/bug/improvement) PR's instead of a larger one. This makes the changes themselves more understandable and reviewable and also gives more room for reverting if something doesn't pan out as thought. It also makes it likely to be a bit easier for newcomers (😇) to contribute quicker.
I do understand that FS has many intertwined complexities which may or may not be easy to untangle in the future. So having small PR's per definition, is something to aspire to.

[yksflip]

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?
Then we can reference this page in the developers tutorial / pullrequest-template

[yksflip]

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.

---

Maintainership

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

• lentschi (flo)
• ray
• robwa
• twothreenine (leo)
• yksflip (philipp)

How to Pull-Request

1. Open a new Pull-Request and reference the Issue it solves. Add a brief description about the changes and if in question any intentions of it.
2. Make sure tests and rubocop are happy.
3. UI changes should be localized in the i18n files directly.
4. Add Screenshots if possible
5. If you don't get any feedback after a week or so, send a ping to any of the maintainers.
6. Small changes can also be fixed by the reviewer directly
7. One approval is required for a PR to be merged

Plugins

When merging a pull request for a plugin, there are some other things to check and do:

• It needs its own LICENSE file, unless it is equal to foodsoft's (GPL3).
• A plugin's locales go into its own config/locales/en.yml (see here). After merging:
• Check that it is not enabled/included in the Gemfile, unless it's a default plugin (like the wiki).
• There is no testing of plugins, yet. When in doubt, checkout and test locally with the plugin enabled.

[RayOei]

One note: I am not a maintainer, only a member. I think that means there are options unavailable to me (maintainer is the owner of the repo if I am correct?). For the PR process it doesn't matter though.

[lentschi]

Thank you @yksflip! 🙇 Looks good to me.

Just one question about a detail:

Once there's one or more approving reviews, who performs the actual merge?
I would say, it should always be the person who created the PR (or if it's a collab of several devs any of those devs). After all they'd be the most involved and might realize they've forgotten something after letting it rest for a bit. Only if they do not merge it for a longer period or if they are no longer available for some reason should the reviewer do the merge.
Do you agree?

[RayOei]

I would agree that the original creator is the first to execute the merge. External contributors are not able to, as far as I am aware, so in that case the person approving can immediately also merge it.
If it is one of the others and it takes a while: ping or merge as I would consider it mergeable when all is green 👍

[RayOei]

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:

• mark with won't fix when it seems the PR will not be accepted, is abandoned, deprecated, etc
  • check also if there is an issue for it and link it if that is not the case
  • check after 4 weeks whether any comments/counterpoints/other are brought in to keep it alive
    • if so: determine how it can be brought back to be worked on and finished
    • if not: reject and close it
• mark with discussion if it seems useful but needs input to be able to finish
  • add on the list for the meetup

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

[lentschi]

I agree with closing stale PRs. 👍

Just to make sure that we're on the same page with your point "check also if there is an issue for it and link it if that is not the case":

I would never close a stale PR without having an active issue that is linked to it, unless it is absolutely certain that we never want it implemented. If there is no issue, it should be created.
When finally closing the stale PR we should probably add a comment like "Possible solution (old): --link to stale PR--" to the issue.
Only that way we can ensure we don't develop things multiple times, because existing (closed) PRs are simply not found.

[RayOei]

The comment for the check is mostly intended as a: "don't forget there should be an issue linked", so like you explain it 😁

[yksflip]

thanks for your additions! I've tried to integrate them into the wiki article. feel free to adapt as you like!