-
-
Notifications
You must be signed in to change notification settings - Fork 711
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
Conversation
/ocabot migration stock_account |
/ocabot merge nobump |
On my way to merge this fine PR! |
@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. |
013afa1
to
abc79c5
Compare
/ocabot merge nobump |
Hey, thanks for contributing! Proceeding to merge this for you. |
@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 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. |
There was a problem hiding this 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
openupgrade_scripts/scripts/stock_account/17.0.1.1/post-migration.py
Outdated
Show resolved
Hide resolved
openupgrade_scripts/scripts/stock_account/17.0.1.1/upgrade_analysis_work.txt
Outdated
Show resolved
Hide resolved
abc79c5
to
e8f87ba
Compare
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:
|
/ocabot merge nobump |
This PR looks fantastic, let's merge it! |
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) |
Congratulations, your PR was merged at e3c853b. Thanks a lot for contributing to OCA. ❤️ |
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. |
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. |
Supersedes #4633.