← Back to team overview

banking-addons-team team mailing list archive

Re: [Merge] lp:~akretion-team/banking-addons/70-banking-addons-fr-lcr into lp:banking-addons

 

Review: Approve

I found your code perfectly fine to look at, but found some minor things that could be fixed even though not impacting functionality:

#80 'this module' (it's just one)
#141 why not payment_order_id?
#253 French or is this a fixed idiom that's not going to be translated anyways?
#820 str.ljust
#821 'the lenght'
#832, #843 everything that's not an acronym should be lower case
#943, #974 why not just concatenations?

And the reason I could read through your code is that I'm a friend of Freanch literature and happen to understand the variable names you chose. It makes it excruciatingly hard for people not speaking the language. I approve, but please, use Esperanto wich is called English nowadays.
-- 
https://code.launchpad.net/~akretion-team/banking-addons/70-banking-addons-fr-lcr/+merge/210701
Your team Banking Addons Core Editors is subscribed to branch lp:banking-addons.


References