Skip to content

[16.0] [MIG] l10n_it_fatturapa#2995

Merged
OCA-git-bot merged 151 commits into
OCA:16.0from
TheMule71:16.0-mig-l10n_it_fatturapa
Dec 16, 2022
Merged

[16.0] [MIG] l10n_it_fatturapa#2995
OCA-git-bot merged 151 commits into
OCA:16.0from
TheMule71:16.0-mig-l10n_it_fatturapa

Conversation

@TheMule71

Copy link
Copy Markdown
Contributor

No description provided.

@TheMule71 TheMule71 mentioned this pull request Nov 2, 2022
81 tasks
<record id="fatturapa_RF02" model="fatturapa.fiscal_position">
<field name="code">RF02</field>
<field
name="name"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sai mica da che configurazione arrivano queste modifiche? A me sembrava più leggibile prima

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Vedi degli errori qui:
https://github.com/OCA/l10n-italy/actions/runs/3381763531/jobs/5616007369
li causa pylint ma non mi chiedere come mai...

@SirTakobi SirTakobi Nov 3, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ah ho trovato, in pratica invece di spostare a sinistra i parent che sono indentati di 8 spazi ma dovrebbero essere indentati solo di 4, sposta solo questi attributi.
Ti ho proposto la modifica in TheMule71#29

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

L'indentazione a 8 deriva dall'eliminazione di <data>. Speravo di minimizzare il diff, facendo vedere solo quello che cambiava ai fini migrazione. In realtà si è messo di mezzo pylint e il risultato finale è così così.
La tua PR va benissimo avevo già in mente di spostare tutto in un commit separato.

Se mi gira potrei spezzare i commit e farne uno --no-verify, e solo dopo reindentare tutto.

@primes2h

primes2h commented Nov 8, 2022

Copy link
Copy Markdown
Contributor

/ocabot migration l10n_it_fatturapa

@OCA-git-bot OCA-git-bot added this to the 16.0 milestone Nov 8, 2022
@TheMule71 TheMule71 force-pushed the 16.0-mig-l10n_it_fatturapa branch 2 times, most recently from 0d522f2 to 5d517ed Compare November 10, 2022 11:14
@TheMule71 TheMule71 marked this pull request as ready for review November 10, 2022 11:14
@TheMule71 TheMule71 force-pushed the 16.0-mig-l10n_it_fatturapa branch 5 times, most recently from 642c0cf to 6adca19 Compare November 11, 2022 08:19
Comment thread l10n_it_fatturapa/views/account_view.xml Outdated
@TheMule71 TheMule71 force-pushed the 16.0-mig-l10n_it_fatturapa branch 4 times, most recently from ba6a425 to cb2140c Compare November 11, 2022 17:04
@TheMule71 TheMule71 requested a review from primes2h November 25, 2022 08:48
@tafaRU

tafaRU commented Nov 25, 2022

Copy link
Copy Markdown
Member

@primes2h puoi aggiornare la review?

@TheMule71

Copy link
Copy Markdown
Contributor Author

@primes2h aspetta che il repo è in disordine... sono tornati dei commit che non ci dovevano essere

@TheMule71 TheMule71 force-pushed the 16.0-mig-l10n_it_fatturapa branch 2 times, most recently from d21f67c to bb82bcc Compare November 25, 2022 11:28
@TheMule71

TheMule71 commented Nov 25, 2022

Copy link
Copy Markdown
Contributor Author

Ok @primes2h vai pure, tieni conto che l'ho rifatta da zero.
Ho anche fatto due commit di 'pre-commit stuff', uno è quello della procedura OCA. Per l'altro ho fatto un pre-commit run -a. Fa un sacco di cose e secondo me l'intenzione della procerura OCA (separare la formattazione dalla migrazione) fallirebbe se quelle modifiche diventassero parte del commit Migration (com'era prima). Purtroppo per poter committare le devi fare prima o poi.
Possiamo discutere se mergiarle in un unico commit.

@TheMule71 TheMule71 force-pushed the 16.0-mig-l10n_it_fatturapa branch from bb82bcc to acc6d02 Compare November 25, 2022 12:18
@primes2h

Copy link
Copy Markdown
Contributor

Ok @primes2h vai pure, tieni conto che l'ho rifatta da zero. Ho anche fatto due commit di 'pre-commit stuff', uno è quello della procedura OCA. Per l'altro ho fatto un pre-commit run -a.

Non ho capito, anche quello della procedura OCA è un pre-commit run -a
https://github.com/OCA/maintainer-tools/wiki/Migration-to-version-16.0#technical-method-to-migrate-a-module-from-150-to-160-branch

Fa un sacco di cose e secondo me l'intenzione della procerura OCA (separare la formattazione dalla migrazione) fallirebbe se quelle modifiche diventassero parte del commit Migration (com'era prima).

Anche qui non mi è chiaro cosa intendi.
Per tenere separata la formattazione dalla migrazione il commit di pre-commit stuff deve essere portato a termine con --no-verify altrimenti (ad es.) ti ritroveresti dentro il cambio di versione del modulo.

Purtroppo per poter committare le devi fare prima o poi. Possiamo discutere se mergiarle in un unico commit.

Personalmente terrei un unico commit con dentro tutte le modifiche richieste (a parte il cambio di versione).
Ora, se non ho capito male, tutti i controlli aggiuntivi di pre-commit (inclusi quelli non estetici) vengono effettuati per tutte le versioni.
A questo punto meglio lasciare tutto dentro il commit iniziale. Tanto prima o poi il codice verrà allineato in automatico.

Comment thread l10n_it_fatturapa/data/fatturapa_data.xml
Comment thread l10n_it_fatturapa/readme/INSTALL.rst Outdated
@TheMule71

Copy link
Copy Markdown
Contributor Author

Ok @primes2h vai pure, tieni conto che l'ho rifatta da zero. Ho anche fatto due commit di 'pre-commit stuff', uno è quello della procedura OCA. Per l'altro ho fatto un pre-commit run -a.

Non ho capito, anche quello della procedura OCA è un pre-commit run -a https://github.com/OCA/maintainer-tools/wiki/Migration-to-version-16.0#technical-method-to-migrate-a-module-from-150-to-160-branch

Si ma il per-commit run -a non fa tutto da solo. Ci sono anche un sacco di warning che non risolve. Al commit successivo, ti blocca se non fai anche quelle cose. Una su tutte, l'eliminazione delle string= quando sono "uguali" al nome python.

Fa un sacco di cose e secondo me l'intenzione della procerura OCA (separare la formattazione dalla migrazione) fallirebbe se quelle modifiche diventassero parte del commit Migration (com'era prima).

Anche qui non mi è chiaro cosa intendi. Per tenere separata la formattazione dalla migrazione il commit di pre-commit stuff deve essere portato a termine con --no-verify altrimenti (ad es.) ti ritroveresti dentro il cambio di versione del modulo.

Sì infatti il primo commit è fatto così. Il problema è che il secondo commit contiene un sacco di cose pre-commit che finirebbero all'interno del commit di migration.

Purtroppo per poter committare le devi fare prima o poi. Possiamo discutere se mergiarle in un unico commit.

Personalmente terrei un unico commit con dentro tutte le modifiche richieste (a parte il cambio di versione). Ora, se non ho capito male, tutti i controlli aggiuntivi di pre-commit (inclusi quelli non estetici) vengono effettuati per tutte le versioni. A questo punto meglio lasciare tutto dentro il commit iniziale. Tanto prima o poi il codice verrà allineato in automatico.

Quella è un'opzione, ma significa non seguire la guida. Significa togliere il --no-verify per il primo commit e mettere tutto il resto fatto a mano lì dentro.

@TheMule71 TheMule71 force-pushed the 16.0-mig-l10n_it_fatturapa branch 3 times, most recently from 8ed12d4 to 9ad5f8a Compare December 6, 2022 13:02
SimoRubi and others added 15 commits December 15, 2022 20:55
Demo data can be loaded when either l10n_generic_coa and l10n_it CoA
are present (or both even), depending on module install order.

We look for both, l10n_it first. Failing that, we search for suitable
accounts (ignoring their names).
Forward port of OCA#2433

Co-authored-by: eLBati <lb@takobi.online>
pylint with optional checks..............................................Passed
- hook id: pylint
- duration: 61.46s

************* Module l10n_it_fatturapa
l10n_it_fatturapa/views/partner_view.xml:51: [W7940(dangerous-view-replace-wo-priority), ]  Dangerous use of "replace" from view with priority 0 < 99
************* Module l10n_it_abicab
l10n_it_abicab/views/abicab_view.xml:29: [W7940(dangerous-view-replace-wo-priority), ]  Dangerous use of "replace" from view with priority 0 < 99
************* Module l10n_it_intrastat_statement.models.intrastat_statement_purchase_section
l10n_it_intrastat_statement/models/intrastat_statement_purchase_section.py:58: [R7980(consider-merging-classes-inherited), IntrastatStatementPurchaseSection1] Consider merging classes inherited to "account.intrastat.statement.purchase.section" from intrastat_statement_purchase_section.py:212, intrastat_statement_purchase_section.py:357, intrastat_statement_purchase_section.py:428.
************* Module l10n_it_intrastat_statement.models.intrastat_statement_sale_section
l10n_it_intrastat_statement/models/intrastat_statement_sale_section.py:26: [R7980(consider-merging-classes-inherited), IntrastatStatementSaleSection1] Consider merging classes inherited to "account.intrastat.statement.sale.section" from intrastat_statement_sale_section.py:172, intrastat_statement_sale_section.py:300, intrastat_statement_sale_section.py:371.

pylint with mandatory checks.............................................Failed
- hook id: pylint
- exit code: 4

************* Module l10n_it_fatturapa
l10n_it_fatturapa/views/partner_view.xml:51: [W7940(dangerous-view-replace-wo-priority), ]  Dangerous use of "replace" from view with priority 0 < 99
************* Module l10n_it_abicab
l10n_it_abicab/views/abicab_view.xml:29: [W7940(dangerous-view-replace-wo-priority), ]  Dangerous use of "replace" from view with priority 0 < 99
Currently translated at 100.0% (234 of 234 strings)

Translation: l10n-italy-14.0/l10n-italy-14.0-l10n_it_fatturapa
Translate-URL: https://translation.odoo-community.org/projects/l10n-italy-14-0/l10n-italy-14-0-l10n_it_fatturapa/it/
@TheMule71 TheMule71 force-pushed the 16.0-mig-l10n_it_fatturapa branch from f377018 to 2f65676 Compare December 15, 2022 19:58
@TheMule71 TheMule71 force-pushed the 16.0-mig-l10n_it_fatturapa branch from 2f65676 to da8647d Compare December 15, 2022 20:33
@TheMule71 TheMule71 requested a review from SirTakobi December 15, 2022 20:34

@primes2h primes2h left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@TheMule71

Copy link
Copy Markdown
Contributor Author

Per non rifare tutto, potresti fare semplicemente cherry-pick del commit della #3004.

Fatto.
Comunque non è così semplice, uno va splittato in 2 perché la #3004 ha due commit, e uno va a toccare sia fatturapa che fatturapa_out. In ogni caso, ho simulato quanto fa il comando di migrazione.

@primes2h

Copy link
Copy Markdown
Contributor

/ocabot merge nobump

@OCA-git-bot

Copy link
Copy Markdown
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 16.0-ocabot-merge-pr-2995-by-primes2h-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit df613e5 into OCA:16.0 Dec 16, 2022
@OCA-git-bot

Copy link
Copy Markdown
Contributor

Congratulations, your PR was merged at e8ed197. 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

Projects

None yet

Development

Successfully merging this pull request may close these issues.