[weboob] [PATCH 2/3] Add module for Ameli website (French Health Insurance)
christophe at lampin.net
Sat Jul 20 23:51:13 CEST 2013
Le 20/07/2013 23:37, Romain Bignon a écrit :
> Nice module. Unfortunately I can't test it, I don't have any credentials for
> this website.
You're not french ?
Ok, but I'll probably need some tests here from others users
> Anyway, I have several questions and comments about your patch:
> On 20/Jul - 22:49, Kitof wrote:
>> delete mode 100755 modules/hellobank/favicon.png
> Why have you included that?
Sorry, a commit mistake.
>> + for detail in self.browser.get_details(subscription):
>> + yield detail
> You can simplify with:
> return self.browser.get_details(subscription)
>> + for bill in self.browser.iter_bills(subscription):
>> + yield bill
>> + def __init__(self, *args, **kwargs):
>> + BaseBrowser.__init__(self, *args, **kwargs)
> Overriding the constructor is useless here as you don't do anything.
>> + def iter_bills(self, sub):
>> + if not sub._id.isdigit():
>> + return 
>> + if not self.is_on_page(BillsPage):
>> + self.location(self.billsp)
>> + self.bills = self.page.get_bills(sub)
>> + return self.bills
> I guess the right test may be "if self.is_on_page(BillsPage)" instead?
> Note that the real name of function may be "get_bills" because you return a list
> instead of a generator.
Ok, i understand now the difference.
>> + def get_bill(self, id):
>> + assert isinstance(id, basestring)
>> + subs = self.get_subscription_list()
>> + for sub in subs:
>> + bills = self.iter_bills(sub)
>> + bill = None
>> + for b in self.bills:
>> + if id == b.id:
>> + return b
> 'bills' and 'bill' variables are useless. That said, it isn't annoying to
> execute this method.
I'll fix that anyway
>> diff --git a/modules/ameli/favicon.png b/modules/ameli/favicon.png
>> new file mode 100755
> A part of the favicon includes a copyright icon, no?
I tried to draw it from the original, maybe it's too close ?
>> +from decimal import *
> You may avoid to include all symbols of a package.
>> + continue
>> + name = " ".join(ident.xpath('.//h4').text.replace(' ', ' ').strip().split())
> You can do something like:
> name = self.parser.tocleanstring(ident.xpath('.//h4'))
>> + number = unicode(ident.xpath('.//li').text).replace(u'Numéro de Sécurité sociale :', '').strip()
> That's always better to use a regexp, or if the number is only composed by
> digits, use something like:
> number = re.sub('[^\d]+', '', ident.xpath('.//li').text)
>> + def get_bills(self,sub):
>> + locale.setlocale(locale.LC_TIME, "fr_FR")
> Never do that in a module, it applies the locale globally, and fr_FR can be
> missing on the running system.
OK, but i need to convert french month name as date, should I write an
Array with all french months to make the translation ?
Is there a way to apply the locale only to a module ?
>> + date = datetime.strptime(date_str, "%B %Y").date()
> It is generally a bad idea to use strptime, I suggest you to see what we do in
> other modules (adecco, ing, americanexpress, cmso, pap). It is often ugly…
OK, I'll have a look.
>> + amount = amount.replace(' euros','')
> You could use re.sub here too I guess (more robust).
> Finally, please use tools/pyflakes.sh before commiting, to find common (minor)
> problems (useless variables, unexistant variables, useless imports, trailing
> spaces, etc.).
Actually I did it, but I have to admit that I'm not familiar with this
tool, and I didn't know what to do with the output. Now, I know ;)
Anyway, thanks for your time, as you can see, python it's not my
favorite language, but I'll correct this and resubmit a patch soon.
More information about the weboob