Review: Disapprove Howdy (I think this is my first code review for you ;-)
- You have defined a html_regexp_link() function. Please write a proper doc string (not some half-assed comment above). - Please name the function correctly; its current name doesn't mean anything. In particular its name should make its intent clear. The fact it uses regex for its job is an implementation detail, no need to state it in its name. - Your variable names are weird: you mix underscore_separated with camelCase, e.g. new_html and innerText. Why not use simple words like original, input, output, processed, expected, and so on. - Your tests seem weird: why does each link end with $@ ? Why does each line end with a . ? - Please think your comments are meant to be read by people who don't know what your code do. For instance # 7. create link would probably read better if it was something along the line of #7. Convert URIs to HTML links. - Instead of a big innerText (together with its outText), you can make a list of pairs and loop over it. Now on the biggest gripe I have: please use a existing tool instead of reinventing the wheel all over again, e.g. https://github.com/jsocol/bleach#readme If you insist on reimplementing stuff on your own, maybe add a proper test suite, see maybe e.g. https://github.com/jsocol/bleach/tree/master/bleach/tests In particular, I would try things like <a> alone, or <href alone, or hellowww.openerp.com you know, weird stuff that people actually write. -- https://code.launchpad.net/~openerp-dev/openobject-server/trunk-text2html-chm/+merge/135607 Your team OpenERP R&D Team is subscribed to branch lp:~openerp-dev/openobject-server/trunk-text2html-chm. _______________________________________________ Mailing list: https://launchpad.net/~openerp-dev-gtk Post to : openerp-dev-gtk@lists.launchpad.net Unsubscribe : https://launchpad.net/~openerp-dev-gtk More help : https://help.launchpad.net/ListHelp
