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

[FIX][account_financial_report_qweb] Fix ImportError. #229

Merged

Conversation

yajo
Copy link
Member

@yajo yajo commented Sep 23, 2016

It was happening when xlsxwriter python module was not available.

@Tecnativa

@@ -50,6 +50,7 @@ Contributors
* Francesco Apruzzese <[email protected]>
* Lorenzo Battistini <[email protected]>
* Julien Coux <[email protected]>
* Jairo Llopis <[email protected]>
Copy link
Member

Choose a reason for hiding this comment

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

This contribution is very little to include you here. In fact, it's a patch.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, but it's mine 😊

Copy link
Member

Choose a reason for hiding this comment

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

git blame will know that, but here we put only relevant contributors, or the list would be big for all the modules.

Copy link
Member Author

Choose a reason for hiding this comment

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

This seems important to me. 😛

@pedrobaeza
Copy link
Member

Bump version number

@sbidoul
Copy link
Member

sbidoul commented Sep 23, 2016

Please, wrap the import xslxwriter at a lower level.

Note that I've never had an import error since I'm using pip to properly install the addons I need.

@yajo yajo force-pushed the 9.0-account_financial_report_qweb-fix_importerror branch from 057f978 to d5e8aa2 Compare September 23, 2016 10:39
yajo added a commit to Tecnativa/docker-ocb that referenced this pull request Sep 23, 2016
"python": [
"xlsxwriter",
],
},
Copy link
Member

Choose a reason for hiding this comment

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

AFAICS xslxwriter is not a direct dependency of afr_qweb. It is a dependency of report_xslx, so it should not be declared here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I updated the patch and put the try lower in the stack. Please review that, you probably missed that.

Copy link
Member

Choose a reason for hiding this comment

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

@yajo indeed I had not seen that part

IMO it is not the best way to do it because this override of create_xlsx_report does not call super(). @jcoux I think reporting-engine must be extended to support options.

Copy link
Member

Choose a reason for hiding this comment

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

And it should be split in 2 modules: one for the QWeb part, and the other for XLSX support extension.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's the job for another PR. 😄

@yajo yajo force-pushed the 9.0-account_financial_report_qweb-fix_importerror branch from d5e8aa2 to 289bc21 Compare September 26, 2016 07:55
It was happening when `xlsxwriter` python module was not available.

Bump version, add external dependencies.
@yajo yajo force-pushed the 9.0-account_financial_report_qweb-fix_importerror branch from 289bc21 to 7eb797a Compare September 26, 2016 07:57
@yajo
Copy link
Member Author

yajo commented Sep 27, 2016

This one should be fast tracked IMHO.

@yajo
Copy link
Member Author

yajo commented Oct 3, 2016

Ping! 😊

@yajo
Copy link
Member Author

yajo commented Oct 11, 2016

💤

@pedrobaeza
Copy link
Member

@sbidoul any feedback on this?

@sbidoul
Copy link
Member

sbidoul commented Oct 12, 2016

@pedrobaeza @yajo I created #241. I'd prefer to get that fixed instead of this PR. What do you think?

@pedrobaeza
Copy link
Member

OK, but if the solution is not still done, I would vote for merging this meanwhile

@sbidoul sbidoul merged commit c4ee0a4 into OCA:9.0 Oct 12, 2016
@yajo yajo deleted the 9.0-account_financial_report_qweb-fix_importerror branch October 13, 2016 09:59
dalonsod pushed a commit to solvosci/account-financial-reporting that referenced this pull request Sep 22, 2022
Signed-off-by pedrobaeza
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.

3 participants