[weboob] [PATCH 2/3] Add module for Ameli website (French Health Insurance)

Christophe Lampin christophe at lampin.net
Sat Jul 20 23:51:13 CEST 2013


Le 20/07/2013 23:37, Romain Bignon a écrit :
> Hello,
>
> 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)
Of course.
>> +            for bill in self.browser.iter_bills(subscription):
>> +                yield bill
> Likewise.
OK
>
>> +    def __init__(self, *args, **kwargs):
>> +        BaseBrowser.__init__(self, *args, **kwargs)
> Overriding the constructor is useless here as you don't do anything.
OK
>
>> +    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.
OK
>
>> +                continue
>> +	
>> +            name = " ".join(ident.xpath('.//h4')[0].text.replace(' ', ' ').strip().split())
> You can do something like:
>              name = self.parser.tocleanstring(ident.xpath('.//h4')[0])
OK
>
>> +                number = unicode(ident.xpath('.//li')[3].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')[3].text)
OK
>
>> +    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).
OK
> 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.

> Romain
Christophe


More information about the weboob mailing list