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

[MIG] [13.0] report_xml: Migrate to version 13.0 #359

Merged
merged 30 commits into from
Apr 24, 2020

Conversation

Tatider
Copy link
Contributor

@Tatider Tatider commented Dec 23, 2019

  • Migrate module
  • Return described but missed fictional of XML validation
    against XSD Schema

@oca-clabot
Copy link

Hey @Tatider, thank you for your Pull Request.

It looks like some users haven't signed our Contributor License Agreement, yet.
You can read and sign our full Contributor License Agreement here: http://odoo-community.org/page/cla
Here is a list of the users:

Appreciation of efforts,
OCA CLAbot

@Tatider
Copy link
Contributor Author

Tatider commented Dec 23, 2019

Linter failed, but I don't agree with suggested changes by next reasons:

  • Some of them will make lines longer that 79 signs (such this one)
  • Most of them suggest changes with out contain comma at the end of line (such this one)
  • Some of them leads to bad readability of code (such this one)

I can make them as a separate commit which message will clear describe, that these are black required changes. But I really would like discuss them with someone. They don't look like good changes for me.

@Tatider Tatider mentioned this pull request Dec 23, 2019
13 tasks
Copy link
Member

@etobella etobella left a comment

Choose a reason for hiding this comment

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

Few comments about what you made...

I think it is a good idea to add the schema, but I do not agree with the implementation.

report_xml/__manifest__.py Show resolved Hide resolved
report_xml/__manifest__.py Outdated Show resolved Hide resolved
report_xml/hooks.py Show resolved Hide resolved
@Tatider Tatider force-pushed the 13.0-mig-report_xml branch 2 times, most recently from c128fbf to 68ecb79 Compare December 24, 2019 09:31
@etobella
Copy link
Member

Linter failed, but I don't agree with suggested changes by next reasons:

* Some of them will make lines longer that 79 signs ([such this one](https://travis-ci.org/OCA/reporting-engine/jobs/628743415#L401))

* Most of them suggest changes with out contain comma at the end of line ([such this one](https://travis-ci.org/OCA/reporting-engine/jobs/628743415#L461))

* Some of them leads to bad readability of code ([such this one](https://travis-ci.org/OCA/reporting-engine/jobs/628743415#L377))

I can make them as a separate commit which message will clear describe, that these are black required changes. But I really would like discuss them with someone. They don't look like good changes for me.

I understand your concerns, but OCA decided to use black in order to make cleaner code. I think you should follow the standard usage, and include this on your commit, as this is part of the standard now.

@Tatider Tatider force-pushed the 13.0-mig-report_xml branch 4 times, most recently from bd7486c to 4263985 Compare December 24, 2019 12:20
@Tatider
Copy link
Contributor Author

Tatider commented Dec 24, 2019

Linter failed, but I don't agree with suggested changes by next reasons:

* Some of them will make lines longer that 79 signs ([such this one](https://travis-ci.org/OCA/reporting-engine/jobs/628743415#L401))

* Most of them suggest changes with out contain comma at the end of line ([such this one](https://travis-ci.org/OCA/reporting-engine/jobs/628743415#L461))

* Some of them leads to bad readability of code ([such this one](https://travis-ci.org/OCA/reporting-engine/jobs/628743415#L377))

I can make them as a separate commit which message will clear describe, that these are black required changes. But I really would like discuss them with someone. They don't look like good changes for me.

I understand your concerns, but OCA decided to use black in order to make cleaner code. I think you should follow the standard usage, and include this on your commit, as this is part of the standard now.

Yes, I understand that it's a part of a standard now, but if the main purpose of using black is make code cleaner, but it doesn't even follow the PEP8...

So:

  1. I changed my code in such way, what I'm almost fine with it, but there were still 2 issues. And I made next step to fix them.
  2. Version of black was updated to the last one, which is 19.10b0 instead of 19.3b0.

And now linters are fine, but build failed. I don't think my changes cause this problem (

@etobella
Copy link
Member

Travis is green, some error was raised on OCB, but it has been fixed now.

@Tatider
Copy link
Contributor Author

Tatider commented Dec 31, 2019

I should upgrade translations files. Old definition of translations for selection fields is deprecated now.

@etobella as I understand from here I can ignore problem with translations, can't I?

@Tatider Tatider force-pushed the 13.0-mig-report_xml branch 3 times, most recently from b21fd68 to f883ac2 Compare January 31, 2020 10:46
Copy link
Member

@Tardo Tardo left a comment

Choose a reason for hiding this comment

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

👍 LGTM only non-blocking comments.

report_xml/controllers/main.py Outdated Show resolved Hide resolved
report_xml/controllers/main.py Outdated Show resolved Hide resolved
report_xml/readme/USAGE.rst Outdated Show resolved Hide resolved
@Tatider Tatider force-pushed the 13.0-mig-report_xml branch 2 times, most recently from de82bae to 98e36f4 Compare February 13, 2020 14:43
.pre-commit-config.yaml Outdated Show resolved Hide resolved
@Tatider Tatider force-pushed the 13.0-mig-report_xml branch 2 times, most recently from 18d498d to cf3c261 Compare February 14, 2020 14:19
@pedrobaeza
Copy link
Member

@etobella please validate the PR and do you agree with the new authorship?

@Tardo
Copy link
Member

Tardo commented Mar 2, 2020

Any progress here?

@sergio-teruel
Copy link

@Tatider CAn you do a rebase to fix isort conficts?? Thanks

Jairo Llopis added 2 commits March 19, 2020 20:31
Almost any XML must start with this. Let's make it easier.
@Tatider
Copy link
Contributor Author

Tatider commented Mar 19, 2020

@Tatider CAn you do a rebase to fix isort conficts?? Thanks

Done

.isort.cfg Outdated Show resolved Hide resolved
@Tatider Tatider force-pushed the 13.0-mig-report_xml branch 2 times, most recently from 785ff04 to dc86d22 Compare March 19, 2020 19:09
@Tardo
Copy link
Member

Tardo commented Apr 21, 2020

This PR need reviews

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

@pedrobaeza pedrobaeza added this to the 13.0 milestone Apr 24, 2020
@pedrobaeza
Copy link
Member

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 13.0-ocabot-merge-pr-359-by-pedrobaeza-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit e901f96 into OCA:13.0 Apr 24, 2020
@OCA-git-bot
Copy link
Contributor

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

None yet