Banking Addons

Merge lp://staging/~akretion-team/banking-addons/bank-statement-reconcile-61-ref-base-completion into lp://staging/banking-addons/bank-statement-reconcile-61

  1. bank-statement-reconcile-61-ref-base-completion
  2. Merge into bank-statement-reconcile-61
Proposed by Benoit Guillot - http://www.akretion.com
Status: Merged
Merged at revision: 80
Proposed branch: lp://staging/~akretion-team/banking-addons/bank-statement-reconcile-61-ref-base-completion
Merge into: lp://staging/banking-addons/bank-statement-reconcile-61
Diff against target: 325 lines (+56/-55)
2 files modified
account_statement_base_completion/statement.py (+51/-50)
account_statement_base_completion/statement_view.xml (+5/-5)
To merge this branch: bzr merge lp://staging/~akretion-team/banking-addons/bank-statement-reconcile-61-ref-base-completion
Reviewer Review Type Date Requested Status
Guewen Baconnier @ Camptocamp code review, no test Approve
Holger Brunn (Therp) code review Approve
Review via email: mp+159105@code.staging.launchpad.net

Description of the change

In this merge proposal, I submit some changes in the module account_statement_base_completion :

 - add a hook for the field function_to_call
 - remove to useless spaces at the end of lines due to my IDE
 - fix typo in the name of the class : AccountBankStatement

The last point can be discuss :

 - don't call automatically the method : get_values_for_line for each statement line that we want to complete

Indeed I think this method should be called in the rule if usefull (it's already the case in the rules already created), but not in every cases.

To post a comment you must log in.
Revision history for this message
Guewen Baconnier @ Camptocamp (gbaconnier-c2c) wrote :

l.289 you added a # TODO FIXME but it doesn't help to know what is actually to fix

l.224,225 if this is to remove, remove it completely instead of comments. I can't say if this is a good change, Nicolas, Joël, do you have any idea?

Thanks

lp://staging/~akretion-team/banking-addons/bank-statement-reconcile-61-ref-base-completion updated
79. By Benoit Guillot - http://www.akretion.com

[FIX] remove comments

Revision history for this message
Benoit Guillot - http://www.akretion.com (benoit-guillot-z) wrote :

l.289 I remove the useless comment, actually it was to discuss this point.
exc.value didn't work but .message worked, do you know why .value didn't worked ?

Revision history for this message
Holger Brunn (Therp) (hbrunn) wrote :

I would expect .value to work in 283, but not in 286. But wouldn't you be better off to use the string representation for both? Then you don't need to know anything about the exception's internals and that's just the idea for the rule that exceptions should have a meaningful string representation. (ErrorTooManyPartner does that)

review: Needs Information
Revision history for this message
Guewen Baconnier @ Camptocamp (gbaconnier-c2c) wrote :

> I would expect .value to work in 283, but not in 286. But wouldn't you be
> better off to use the string representation for both? Then you don't need to
> know anything about the exception's internals and that's just the idea for the
> rule that exceptions should have a meaningful string representation.
> (ErrorTooManyPartner does that)

Holger is right, as of python 2.6, BaseException.message has been deprecated and should be replaced by the string representation of the exception instead.

review: Needs Fixing
lp://staging/~akretion-team/banking-addons/bank-statement-reconcile-61-ref-base-completion updated
80. By Benoit Guillot - http://www.akretion.com

[FIX] use string representation of the exception

Revision history for this message
Benoit Guillot - http://www.akretion.com (benoit-guillot-z) wrote :

I have changed the exception message, know I use the string representation.
Thanks for your comments and explanations !

Revision history for this message
Holger Brunn (Therp) (hbrunn) :
review: Approve (code review)
Revision history for this message
Guewen Baconnier @ Camptocamp (gbaconnier-c2c) wrote :

LGTM

review: Approve (code review, no test)
Revision history for this message
Guewen Baconnier @ Camptocamp (gbaconnier-c2c) wrote :

Merged.

Benoit, did you created the MP for the version 7 somewhere?

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches