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.

Sign up for GitHub

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

Jump to bottom

Migration 13.0 -> 14.0: Substitute act_window and report with proper record tags #38

Merged
merged 3 commits into from
Feb 1, 2021
Merged

Migration 13.0 -> 14.0: Substitute act_window and report with proper record tags #38

merged 3 commits into from
Feb 1, 2021

Conversation

Daemo00
Copy link
Contributor

@Daemo00 Daemo00 commented Oct 24, 2020

Adding a bit of Python automation for said part of 14.0 migration

@Daemo00 Daemo00 mentioned this pull request Oct 24, 2020
Closed
@Daemo00 Daemo00 force-pushed the 13.0-14.0-migrate_deprecated_tags branch from 905d9fb to 763ff10 Compare October 24, 2020 20:24
@Daemo00 Daemo00 force-pushed the 13.0-14.0-migrate_deprecated_tags branch from 3823f51 to 3586a0e Compare October 26, 2020 18:53
@Daemo00 Daemo00 force-pushed the 13.0-14.0-migrate_deprecated_tags branch from 3586a0e to 872c53c Compare November 6, 2020 22:41
@yelizariev yelizariev mentioned this pull request Nov 7, 2020
Merged
@Daemo00
Copy link
Contributor Author

Daemo00 commented Dec 13, 2020

@legalsylvain can this be merged?

@yelizariev yelizariev mentioned this pull request Jan 14, 2021
Merged
@Daemo00 Daemo00 mentioned this pull request Jan 16, 2021
Closed
@Daemo00 Daemo00 force-pushed the 13.0-14.0-migrate_deprecated_tags branch from 5ec5747 to d6d3b31 Compare January 16, 2021 21:18
@Daemo00
Copy link
Contributor Author

Daemo00 commented Jan 16, 2021

Rebased due to conflicts with master and added a little fix in last commit for the loading of migration scripts

@Daemo00 Daemo00 force-pushed the 13.0-14.0-migrate_deprecated_tags branch from 07aaf75 to e1143d5 Compare January 17, 2021 14:28
yelizariev
yelizariev approved these changes Jan 19, 2021
View reviewed changes
Copy link
Member

@yelizariev yelizariev left a comment

Choose a reason for hiding this comment

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

You did a great work!

Commits history could be merged to make this PR perfect.

Thanks for your efforts

odoo_module_migrate/migration_scripts/migrate_130_140.py
if match:
indent = match.group('indent')
tag_match = match.group('tag')
et.indent(tag, space=indent, level=1)
Copy link
Member

Choose a reason for hiding this comment

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

You even keep original indent. Perfect 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

@yelizariev Why do we have to be so careful to only format the modified piece of code, if we then have to pass the "black" tool to format all the files?

https://github.com/OCA/maintainer-tools/wiki/Migration-to-version-14.0#howto
pre-commit run -a # to run black, isort and prettier (ignore pylint errors at this stage)

Copy link
Member

Choose a reason for hiding this comment

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

Because it's easier to check migration changes.
Also, you may want don't run black formating to avoid changes in git history (remember, that this tool can be used outside of OCA)

odoo_module_migrate/migration_scripts/migrate_130_140.py Show resolved Hide resolved
@Daemo00 Daemo00 force-pushed the 13.0-14.0-migrate_deprecated_tags branch from 7bef232 to 35a4b64 Compare January 19, 2021 21:32
@Daemo00
Copy link
Contributor Author

Daemo00 commented Jan 19, 2021

You did a great work!

Commits history could be merged to make this PR perfect.

Thanks for your efforts

Yes this wasn't easy but there was something that I had to learn :)

I reorganized the commit history:

  • First modification
  • A commit you co-authored -> can't squash
  • More modifications, can't squash with first part because of ^
  • Generic fix to migration script
  • Another generic fix to migration script

The only other configuration I can see is to squash the first 3 commits but then you'd lose authorship, let me know if that's ok

@yelizariev
Copy link
Member

@Daemo00 I'm ok with squashing first 3 commits and losing authorship 😉
I don't even remember why I'm there 😄
My reviews in this PR is enough to document my contribution to this work 😊

@joao-p-marques
Copy link
Member

@Daemo00 you can still keep the authorship by adding @yelizariev as a co-author to the commit when rebasing: https://github.blog/2018-01-29-commit-together-with-co-authors/
Just a suggestion ☺️
Great work BTW! 👍

@Daemo00 Daemo00 force-pushed the 13.0-14.0-migrate_deprecated_tags branch from 35a4b64 to e11d78e Compare January 20, 2021 18:27
@Daemo00
Copy link
Contributor Author

Daemo00 commented Jan 20, 2021

@Daemo00 I'm ok with squashing first 3 commits and losing authorship
I don't even remember why I'm there
My reviews in this PR is enough to document my contribution to this work

@yelizariev then here you have the 3 commits nice and clean

@yelizariev
Copy link
Member

@legalsylvain this adds new script and fix the tool in general

@Daemo00
Copy link
Contributor Author

Daemo00 commented Jan 27, 2021

Can this be merged?

@yelizariev
Copy link
Member

@ivantodorovich could you review?

FernandoRomera
FernandoRomera approved these changes Jan 28, 2021
View reviewed changes
Copy link
Contributor

@FernandoRomera FernandoRomera left a comment

Choose a reason for hiding this comment

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

Review, ok.

@OCA-git-bot
Copy link
Contributor

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). 🤖

@FernandoRomera
Copy link
Contributor

@legalsylvain Can you merge this PR, please?.

@legalsylvain
Copy link
Collaborator

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch master-ocabot-merge-pr-38-by-legalsylvain-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit f0a3e30 into OCA:master Feb 1, 2021
@OCA-git-bot
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yelizariev yelizariev yelizariev approved these changes

@joao-p-marques joao-p-marques joao-p-marques left review comments

@AntoniRomera AntoniRomera AntoniRomera approved these changes

@FernandoRomera FernandoRomera FernandoRomera approved these changes

Assignees
No one assigned
Labels
approved merged 🎉 ready to merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@Daemo00 @yelizariev @joao-p-marques @OCA-git-bot @FernandoRomera @legalsylvain @AntoniRomera