Code review comment for lp:~pedro.baeza/account-financial-tools/7.0-account_chart_update-enhanced
Revision history for this message
| Yannick Vaucher @ Camptocamp (yvaucher-c2c) wrote | # |
review:
Needs Fixing
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
a60fb26
(Get the code!)
Ok thanks for the changes.
About the hack for required=true, please add a #FIXME comment if there is no other option. And checking chart_template_id is set in
_map_tax_
_find_tax_codes
_find_accounts
Seams necessary.
To respect PEP8 80 cols I don't feel it is the right way to go. And it creates more diff than necessary. But I won't block it for that.
There are still some issue with PEP8, indentation and length of lines introduced by your changes:
l.1533
l.1903
l.1940
l.2160
l.1351 missing space after comma