Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[17.0][OU-ADD] stock_account: Migration to 17.0 #4733

Merged
merged 2 commits into from
Feb 10, 2025

Conversation

MiquelRForgeFlow
Copy link
Contributor

Supersedes #4633.

@MiquelRForgeFlow
Copy link
Contributor Author

/ocabot migration stock_account

@MiquelRForgeFlow
Copy link
Contributor Author

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 17.0-ocabot-merge-pr-4733-by-MiquelRForgeFlow-bump-nobump, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Feb 6, 2025
Signed-off-by MiquelRForgeFlow
@OCA-git-bot
Copy link
Contributor

@MiquelRForgeFlow your merge command was aborted due to failed check(s), which you can inspect on this commit of 17.0-ocabot-merge-pr-4733-by-MiquelRForgeFlow-bump-nobump.

After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red.

@MiquelRForgeFlow
Copy link
Contributor Author

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 17.0-ocabot-merge-pr-4733-by-MiquelRForgeFlow-bump-nobump, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Feb 6, 2025
Signed-off-by MiquelRForgeFlow
@OCA-git-bot
Copy link
Contributor

@MiquelRForgeFlow your merge command was aborted due to failed check(s), which you can inspect on this commit of 17.0-ocabot-merge-pr-4733-by-MiquelRForgeFlow-bump-nobump.

After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red.

@hbrunn
Copy link
Member

hbrunn commented Feb 6, 2025

@MiquelRForgeFlow I don't think it's great style to close another contributor's PR without comment, and then go ahead and (try to) merge your own version without review.

I'll check next week if I can help with the failing tests.

Copy link
Member

@hbrunn hbrunn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tests are fixed in #4734

@MiquelRForgeFlow
Copy link
Contributor Author

MiquelRForgeFlow commented Feb 10, 2025

I don't think it's great style to close another contributor's PR without comment, and then go ahead and (try to) merge your own version without review.

Short answer: I think this repository never went for great style. I think it's more like a far west, at least that's what @pedrobaeza says sometimes.

Longer answer:

  • The contributor code was saved in their own commit, I respected that, so everyone can track their contribution.
  • Whether it's better to supersede the PR or request for changes and wait for the changes to be handled by the contributor, it's a fine line. My logic is this: If I feel the contributor is someone active, that can handle changes in a fast way, then I go for the request for changes. But if the PR is old enough, and I don't know the contributor or feel they won't handle fast enough the changes, then I go for the superseded. I don't think a contributor should get annoyed by a supersede.
  • As to why I try to automerge my PR, well, it's because in some cases, like this one, I feel that the changes I am adding are safe enough and thus I don't need a second review to proceed with the merge. We are managers of this repository for some reason.

@MiquelRForgeFlow
Copy link
Contributor Author

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 17.0-ocabot-merge-pr-4733-by-MiquelRForgeFlow-bump-nobump, awaiting test results.

@hbrunn
Copy link
Member

hbrunn commented Feb 10, 2025

well, I disagree. Demanding reviews is part of the rules we gave ourselves, it's a super bad look when people who can skirt those rules. I think the only acceptable exception to this are administrative things like making tests work.

As for other people's PRs: When no review happened for months, and then a maintainer goes and just supersedes something, even with preserving commits, it's insulting in my opinion. I've done the same in the past, but reassessed: we need to treat contributions with respect if we want more of them (of course I know, there are contributions that are just bad and it's hard to even get started describing what's all wrong with it, and there are obvious low effort PRs that are an insult to the reviewer etc. But none of that was the case here)

@OCA-git-bot OCA-git-bot merged commit e3c853b into OCA:17.0 Feb 10, 2025
1 of 2 checks passed
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at e3c853b. Thanks a lot for contributing to OCA. ❤️

@MiquelRForgeFlow MiquelRForgeFlow deleted the 17.0_ou_stock_account branch February 10, 2025 15:57
@pedrobaeza
Copy link
Member

pedrobaeza commented Feb 10, 2025

Hey, wait a minute. I have never said nothing about Far West (or not seriously). My statement is that when a work is not completed, and seen that the contributors are not around now, instead of putting comments that is not going to be attended, it's preferable to supersede the work (respecting the attribution) to finish it.

Said that, I think this is the case and the supersede is legit, as Microcom already expressed that they are not working now on OU (not finding right now the comment where Alexis said it, sorry). @hbrunn, you did something similar in #4557 (comment)

About auto-merging or to not wait 2 approvals, we agreed in the past with @StefanRijnhart about doing that on this special project, while we are confident about the result.

@StefanRijnhart
Copy link
Member

I know I am not active on this repository as a reviewer, but since I was mentioned: I think new or occasional contributors could be stimulated to increase their contributions if we give proper feedback and give them a couple of days to get back. And increased contributions from more people would be welcome here, I hope.

On the other point: I agree that waiting for a second approval is not practical on this project, or was not practical before the OCA picked this project for paid contributions, as I hope that this also improves things. Then again, to self-merge without any review is taking it one or two steps further.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants