search

OCA / delivery-carrier

Odoo Carriers And Deliveries Management
https://odoo-community.org/psc-teams/logistics-18
GNU Affero General Public License v3.0
112 stars 351 forks source link

[14.0][FIX] delivery_auto_refresh #849

Open toita86 opened 2 months ago

toita86 commented 2 months ago

backport of https://github.com/OCA/delivery-carrier/pull/799

OCA-git-bot commented 2 months ago

Hi @aleuffre, @PicchiSeba, @renda-dev, some modules you are maintaining are being modified, check this out!

francesco-ooops commented 2 months ago

@jbaudoux would you mind reviewing this backporting? thanks!

jbaudoux commented 2 months ago

cc @pedrobaeza

OCA-git-bot commented 2 months ago

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

toita86 commented 1 month ago

Can you please squash a bit the commit history?

@pedrobaeza Of course, do you have any suggestion? I'm not to sure on how to do it.

francesco-ooops commented 1 month ago

Or rather, which commits to squash together

rousseldenis commented 1 month ago

I would say commits should remain as it is. They reflect development flow and are the same on the other main branch (even if they should have been a little bit enhanced in their messages).

pedrobaeza commented 1 month ago

No, they don't add value when merged. The commit "delivery_auto_refresh: fix create in batch" is not adding value to have it separated. The code itself speaks by itself. And they are not following commit message guidelines:

https://github.com/OCA/odoo-community.org/blob/master/website/Contribution/CONTRIBUTING.rst#commit-message

francesco-ooops commented 1 month ago

@pedrobaeza can you please tell us in detail which ones should be squashed in your opinion, so we can proceed?

rousseldenis commented 1 month ago

@jbaudoux

rousseldenis commented 1 month ago

@pedrobaeza I would say you had merged it yousrself like this : https://github.com/OCA/delivery-carrier/pull/799#pullrequestreview-2118683894

pedrobaeza commented 1 month ago

IMO, you can pack together improvements which code speaks by itself, summarizing in the commit message all of them. At the end, what we want when we do a blame, is to identify the reason of the change. But if there are several commits touching the same lines of code, at the end, the blame operation is a hell.

The same way, a commit that is not telling the reason behind, doesn't have value too much value. Take this commit:

https://github.com/OCA/delivery-carrier/pull/849/commits/4785c77d8d28b207cd74085e03ef2336e610ea32

With the commit message delivery_auto_refresh: method copied from 16.0. Which value brings to have it separated if it's not bringing any useful information, and even more, it's not being called in any place in the same commit!!! It may be interesting to keep it as a separated commit if at least where it's used is packed in the same commit and there's an explanation of why this is done.

I know that it seems comfortable to do continuously git commit to "save the current work", but at the end, that doesn't help when reviewing code (as our development cycle may not respond the feature cycle), and also for grouping changes in a logical way.

For reference, what I do to save the work is usually git commit --amend. If working in several features at the same time, I put several commits, and later changes are committed with --fixup <sha> for merging them before pushing with the proper original commits.

pedrobaeza commented 1 month ago

@pedrobaeza I would say you had merged it yousrself like this : #799 (review)

Yes, as said in other thread, I don't fight all the possible battles. I have to resign in some of them. In this case, it was my fault to block the PR a lot of time and I already asked for a lot of changes, so I didn't want to be even more picky about that.

In this case, it's also not the most important thing, as this is an old branch, but the number of commits have increased (from 11 to 16), and it can be an opportunity to converge in commit strategies.

francesco-ooops commented 1 month ago

@pedrobaeza in the case of commit delivery_auto_refresh: method copied from 16.0 it was isolated in a specific commit to preserve odoo SA authorship. Should we also keep commit name/description, even if it's not the full content of the commit?

pedrobaeza commented 1 month ago

Good that you have arisen this, as I wasn't really aware (didn't check this PR in content yet). Starting with, you can't do that, as this module is AGPL, and the original code is LGPL. More in the formal aspect, you are putting that method, but not indicating anything in the commit message saying that this is copied from Odoo code, and not putting in the same commit the changes on the README Part of this module is a backport.... That's what I refer about packing things in a logical way in the commits, and also to document things properly for helping others (or ourselves in the future) to understand why things are there.

About the code itself, I don't think the prepare method is bringing too much value, and even if so, you can just put it without that attribution concern, as at the end, it's not a specific secret technique to do the code, and the lines are obvious even and will be the same if you do it by hand, and for sure you need later adjustments for version 14.