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

[12.0][FIX] Fixed wizard_base_mixin default get #1930

Merged
merged 1 commit into from
May 28, 2022

Conversation

hirokibastos
Copy link
Contributor

Primero PR de trabalho em progresso para dividir #1701

Fix simples de bug no default_get do wizard_base_mixin

@OCA-git-bot
Copy link
Contributor

Hi @renatonlima,
some modules you are maintaining are being modified, check this out!

hirokibastos added a commit to kmee/l10n-brazil that referenced this pull request May 27, 2022
@marcelsavegnago
Copy link
Member

@hirokibastos parabens.. esse é raciocinio. A ideia é tentar fatiar bem as coisas porque isso facilita inclusive a busca por problemas nas PRs.

@rvalyi
Copy link
Member

rvalyi commented May 27, 2022

@hirokibastos parabens.. esse é raciocinio. A ideia é tentar fatiar bem as coisas porque isso facilita inclusive a busca por problemas nas PRs.

exactamente. Porque imagina que na hora de migrar para a v18, alguem tanta migrar um modulo como sei la l10n_br_account_nfe e se depara com essa linha e pensa que nao deve servir para nada e se nao seria bom remover:


ai o cara da um git blame, nisso ele encontra o PR com a explicação do Renato, vê quem aprovou, e vê que é importante (ou sabe quem deve explanar para ver se para de fazer merda). Agora se em vez disso ele cair num PR de 1000 linhas de diff onde tem 3 iniciantes que fizeram 60 commits cheio de 3 passos para frente e 2 passos para trás, ai já era; ninguém não consegue buscar mais nada e não fica digno de qualidade OCA.

@marcelsavegnago
Copy link
Member

marcelsavegnago commented May 27, 2022

@hirokibastos parabens.. esse é raciocinio. A ideia é tentar fatiar bem as coisas porque isso facilita inclusive a busca por problemas nas PRs.

exactamente. Porque imagina que na hora de migrar para a v18, alguem tanta migrar um modulo como sei la l10n_br_account_nfe e se depara com essa linha e pensa que nao deve servir para nada e se nao seria bom remover:

ai o cara da um git blame, nisso ele encontra o PR com a explicação do Renato, vê quem aprovou, e vê que é importante (ou sabe quem deve explanar para ver se para de fazer merda). Agora se em vez disso ele cair num PR de 1000 linhas de diff onde tem 3 iniciantes que fizeram 60 commits cheio de 3 passos para frente e 2 passos para trás, ai já era; ninguém não consegue buscar mais nada e não fica digno de qualidade OCA.

Sim.. no minimo se a PR tem o objetivo de implementar novas funcionalidades, as correções devem ser feitos em outras PRs.

Sei que em alguns casos estamos ali na imersão total tentando fazer algo rodar e tal.. nao tem problema fazer os ajustes necessáriso em N modulos ou para resolver N situações como por exemplo um erro. O esquema eh ir comitando separadinho, separar sempre as PRs por módulo e se possível no caso de um erro que foi encontrado durante o desenvolvimento já ir criando uma branch o quanto antes, fazer o cherry pick e submeter PR para os camaradas :D.

Fatiar o boi eh preciso :D Dói no começo mas depois fica gostoso 😄

@marcelsavegnago
Copy link
Member

@hirokibastos Pergunta besta :D não fica triste.. Vcs estão com esse bug guardado ai tem quanto tempo ?? :D

@rvalyi
Copy link
Member

rvalyi commented May 28, 2022

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 12.0-ocabot-merge-pr-1930-by-rvalyi-bump-patch, awaiting test results.

OCA-git-bot added a commit that referenced this pull request May 28, 2022
Signed-off-by rvalyi
@OCA-git-bot
Copy link
Contributor

@rvalyi your merge command was aborted due to failed check(s), which you can inspect on this commit of 12.0-ocabot-merge-pr-1930-by-rvalyi-bump-patch.

After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red.

@rvalyi
Copy link
Member

rvalyi commented May 28, 2022

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 12.0-ocabot-merge-pr-1930-by-rvalyi-bump-patch, awaiting test results.

OCA-git-bot added a commit that referenced this pull request May 28, 2022
Signed-off-by rvalyi
@OCA-git-bot
Copy link
Contributor

@rvalyi your merge command was aborted due to failed check(s), which you can inspect on this commit of 12.0-ocabot-merge-pr-1930-by-rvalyi-bump-patch.

After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red.

@rvalyi
Copy link
Member

rvalyi commented May 28, 2022

erros no l10n_br_website_sale para variar...
/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 12.0-ocabot-merge-pr-1930-by-rvalyi-bump-patch, awaiting test results.

OCA-git-bot added a commit that referenced this pull request May 28, 2022
Signed-off-by rvalyi
@OCA-git-bot
Copy link
Contributor

@rvalyi your merge command was aborted due to failed check(s), which you can inspect on this commit of 12.0-ocabot-merge-pr-1930-by-rvalyi-bump-patch.

After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red.

@rvalyi
Copy link
Member

rvalyi commented May 28, 2022

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 12.0-ocabot-merge-pr-1930-by-rvalyi-bump-patch, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 9ef53d2 into OCA:12.0 May 28, 2022
@OCA-git-bot
Copy link
Contributor

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

hirokibastos added a commit to kmee/l10n-brazil that referenced this pull request Jun 2, 2022
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.

5 participants