[MIG] [16.0] product sticker

[Shide]

Standard migration, added @rafaelbn as a maintainer too.

MT-6566 @moduon @rafaelbn @yajo @fcvalgar @Gelojr please review if you want :)

[fcvalgar]

Standard migration, nothing changed.

Moduon - Task #6609 @moduon @rafaelbn @yajo @fcvalgar @Gelojr please review if you want :)

Internal task MT-6566

[fcvalgar]

Functional review,

LGTM thank you @Shide

[Gelojr]

LGTM

[rafaelbn]

/ocabot merge patch

[OCA-git-bot]

Sorry @rafaelbn you are not allowed to merge.

To do so you must either have push permissions on the repository, or be a declared maintainer of all modified addons.

If you wish to adopt an addon and become it's maintainer, open a pull request to add your GitHub login to the maintainers key of its manifest.

[rafaelbn]

• @Shide we should be maintainers of this module please! 🙏🏼 😄 thank you! ❤️
• After merge please review [MIG] [16.0] stock_picking_report_product_sticker stock-logistics-reporting#330

[Shide]

@etobella can you review and merge this one? You merge it on v15. Thanks! ❤️

[Shide]

@rousseldenis please can you merge this one? I'm the maintainer on v15. Thanks! ❤️

[rafaelbn]

Hello @OCA/product-maintainers ,

Please could you merge this PR? This is a direct migration from v15 of a module that we are maintainers 😄 :https://github.com/OCA/product-attribute/blob/15.0/product_sticker/__manifest__.py#L21

Standard migration, added @rafaelbn as a maintainer too.

We have also pending [MIG][16.0] stock_picking_report_product_sticker wich depends on this one!

Thank you! ❤️ 🙏🏼

[yajo]

@Shide you're the maintainer in v15:

product-attribute/product_sticker/__manifest__.py

Line 21 in 16284af

"maintainers": ["Shide"],

Please try to issue the merge command yourself. The bot should obey you if you're the previous version maintainer.

[Shide]

/ocabot merge nobump

[OCA-git-bot]

Sorry @Shide you are not allowed to merge.

To do so you must either have push permissions on the repository, or be a declared maintainer of all modified addons.

If you wish to adopt an addon and become it's maintainer, open a pull request to add your GitHub login to the maintainers key of its manifest.

[rafaelbn]

Please @gurneyalex @legalsylvain could you please make a quick review here and merge? 🙏🏼 This is a direct migration of a module that we are maintainers 😄 ❤️

[HaraldPanten]

@Shide Aren't you mixing the pre-commit changes with the migration commit?

[Shide]

@HaraldPanten Yes, I'm mixing it because pre-commit doesn't give us relevant information

[etobella]

@Shide pre-commit may not give relevant information, but simplifies review, it exists for reviewers 😉. Next time try to follow OCA rules please

[HaraldPanten]

@HaraldPanten Yes, I'm mixing it because pre-commit doesn't give us relevant information

Hi Shide. I know, but pre-commit message has to be separate from migration. If you do it, I'll merge your PR ASAP 👍

[yajo]

Hi folks. When splitting the pre-commit commit, there's something you have to take into account. When the contributor changes the version or maintainers in the manifest, just doing git commit will rewrite the README. That's the thing that's happening here. This is because the README generator takes the manifest into account when deciding if it should update the README.

A very different thing is when, before doing that, the contributor just runs pre-commit run -a and saves those automated reformats in a separate commit. However, that's not something applicable to this PR because if you do that before changing anything, there won't be any changes anyways.

In short, if you follow the recommended steps, you'll end up with a PR exactly like this one. Therefore, it's ready to merge.

[pedrobaeza]

That's not true, @yajo. If you follow the guide you have linked (which is the official OCA guide for this), you end up with 2 commits, not one. The flag --no-verify on the commit command for the pre-commit auto-fixes makes that no additional changes are required to be done in the first commit due to linters.

If you want to change the current OCA guidelines, please raise an issue or mail on the usual channels to ask for that change. Meanwhile, you must follow the procedure and put the 2 commits, so @HaraldPanten's claim is totally legit.

[HaraldPanten]

Hi folks. When splitting the pre-commit commit, there's something you have to take into account. When the contributor changes the version or maintainers in the manifest, just doing git commit will rewrite the README. That's the thing that's happening here. This is because the README generator takes the manifest into account when deciding if it should update the README.

A very different thing is when, before doing that, the contributor just runs pre-commit run -a and saves those automated reformats in a separate commit. However, that's not something applicable to this PR because if you do that before changing anything, there won't be any changes anyways.

In short, if you follow the recommended steps, you'll end up with a PR exactly like this one. Therefore, it's ready to merge.

Hi Jairo, @rafaelbn @Shide My only point here is trying to help you to move forward with this PR. All migrations are separating pre-commit stuff vs migration commit. That's why I asked you to do that.

You can see that we are doing this as well in every migration we do:

OCA/timesheet#701
OCA/project#1319
OCA/web#2897
...

Once done, I'll merge this module. 🤙

[HaraldPanten]

/ocabot migration product_sticker

[Shide]

@HaraldPanten I promise next migration PRs won't merge the pre-commit message.

But, I'm the maintainer and no one has modified this module in v15, so the commit author has been respected.

I don't see any point to make me work more undoing a migration and make it again to have the same result.

I promise again in the next migration PRs won't merge the pre-commit message, but for now, I think it's enough work for that PR series.

Thanks in advance!

[HaraldPanten]

@HaraldPanten I promise next migration PRs won't merge the pre-commit message.

But, I'm the maintainer and no one has modified this module in v15, so the commit author has been respected.

I don't see any point to make me work more undoing a migration and make it again to have the same result.

I promise again in the next migration PRs won't merge the pre-commit message, but for now, I think it's enough work for that PR series.

Thanks in advance!

Hi @Shide I'm not sure if I understood you well.

As far as I know, this is a standard migration. The only change is that you're adding Rafa as a maintainer. That's OK.

But... Why is it a problem to run pre-commit and have separate changes (pre-commit vs migration) in different commits?

It's not about me (or my rules), it's because everybody is doing the same in all OCA. And I'm asking these changes in every Migration PR I review. Please, have a look at --> OCA/account-financial-tools#1819 (comment)

[Shide]

Closed in favor of #1698