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

Romain Bignon romain at symlink.me
Sat Jul 20 23:37:58 CEST 2013


Hello,

Nice module. Unfortunately I can't test it, I don't have any credentials for
this website.

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?

> +            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

Likewise.

> +    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.

> +    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.

> 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?

> +from decimal import *

You may avoid to include all symbols of a package.

> +                continue
> +	
> +            name = " ".join(ident.xpath('.//h4')[0].text.replace(' ', ' ').strip().split())

You can do something like:
            name = self.parser.tocleanstring(ident.xpath('.//h4')[0])

> +                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)

> +    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.

> +            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…

> +            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.).

Romain
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 490 bytes
Desc: Digital signature
URL: <http://lists.symlink.me/pipermail/weboob/attachments/20130720/aa00f79b/attachment.pgp>


More information about the weboob mailing list