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

[ADD] add option rename-to to allow rename addon #64

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

trisdoan
Copy link
Contributor

@trisdoan trisdoan commented Dec 18, 2024

This change

@trisdoan trisdoan marked this pull request as draft December 18, 2024 11:24
@trisdoan trisdoan changed the title [WIP][ADD] add option rename-to to allow rename add [WIP][ADD] add option rename-to to allow rename addon Dec 18, 2024
@trisdoan trisdoan force-pushed the add-rename-to-option branch from dc1f63f to f5d3ad4 Compare December 20, 2024 09:43
@trisdoan trisdoan changed the title [WIP][ADD] add option rename-to to allow rename addon [ADD] add option rename-to to allow rename addon Dec 20, 2024
@trisdoan trisdoan marked this pull request as ready for review December 20, 2024 10:02
@trisdoan trisdoan marked this pull request as draft December 20, 2024 10:15
oca_port/migrate_addon.py Outdated Show resolved Hide resolved
oca_port/migrate_addon.py Outdated Show resolved Hide resolved
oca_port/migrate_addon.py Outdated Show resolved Hide resolved
def _rename_module(self):
if self.app.rename_to:
try:
import git_filter_repo as fr
Copy link
Collaborator

Choose a reason for hiding this comment

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

I remember a command that was in the OCA documentation in the past to rework history to rename a module, I can't find it anymore...
I tend to reduce the dependencies if possible, especially if a git command can do the job.

Keep this lib for now, I still didn't test your PR :)

And for such feature (renaming a module), we can consider it as required, so such dependency should be in the pyproject.toml (and try/except block can be removed).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you mean: git filter-branch --tree-filter 'if [ -d old_module_name ]; then mv old_module_name new_module_name; fi' HEAD , right?

Now I am thinking git-filter-repo could be overkill for this case, let me experiment with the git filter-branch

Copy link
Collaborator

@sebalix sebalix Jan 2, 2025

Choose a reason for hiding this comment

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

I guess, I don't remember exactly 🤷‍♂️

Open questions:

  • do we want to rename the module within all the history? (I don't know what is the "best practice" regarding this in OCA)
  • is PortAddonPullRequest working as expected when porting commits from module A which has been renamed B on target branch? (BTW having tests for such feature and this use case is welcome)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is PortAddonPullRequest working as expected when porting commits from module A which has been renamed B on target branch? (BTW having tests for such feature and this use case is welcome)

You mentioned the point I did not notice, will add some tests

@sebalix
Copy link
Collaborator

sebalix commented Dec 20, 2024

Thanks for this work 🙏 I wanted to start working on it yesterday and saw your PR 🎉 I'll test it soon

@trisdoan
Copy link
Contributor Author

trisdoan commented Dec 25, 2024

I notice that current approach does not work when module locates in subfolder

I am working on it, will ping you when it's done

@trisdoan trisdoan force-pushed the add-rename-to-option branch from f5d3ad4 to 568a3af Compare December 25, 2024 10:02
@trisdoan trisdoan marked this pull request as ready for review December 25, 2024 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants