Review: Needs Fixing
Python
* Unneeded import: StringIO is imported in Import.detect_data, not even used
* Unused local variables: `all_fields` is created in Import.detect_data
(main.py:1378), this value is never used because it's replaced completely a few
lines below (main.py:1378)
* Redundant code: the result of `fields_get` is passed to `dict`. As far as I
know it's already a dict, why re-dict it? This happens at both main.py:1379 and
main.py:1497
* The reuse of `fields` in detect_data to store two pretty different things is
weird
* In Import.import_data, StringIO is only used to put the contents of a file
in, and pass *that* to csv.reader. Why not just use the file itself?
* Rather than use enumerate and check for a given number before breaking off,
why not use slices or `itertools.islice` to limit the size of the iteration to
whatever it is you want or need?
* If you want the first record of an iterator, you can just use `.next()` on it
(or `iter(iterable).next()` just in case the object is an iterable but not an
iterator), no need to iterate on the whole thing and break after the first
round (or whatever), this weird pattern is done in both Import.detect_data and
Import.import_data
* main.py:1384, `dict.get` returns None if the key is not found, so a default
value of `False` in a boolean test is redundant and worthless. And so is
testing against `True`, at least in this precise case. And the parens are
redundant here. Why not just write `if value.get('required'):`? There's
something similar at main.py:1405 (`None` is not equal to `'one2many'` so why
provide an empty string default instead?)
* main.py:1507, why do you get the dict keys to test if the dict has (or
doesn't have) a given key? You can just use `key in dict`. Plus it's probably
faster: containment test is O(1) on hashmaps (which python dicts are) and O(n)
on arrays (which python lists are), and you have to pay the cost of allocating
and filling a list.
- Plus there are redundant parens.
* Why do you use kwargs everywhere? They're not useful, why not use *normal*
parameters? Especially since most of the parameters are *not* optional (what's
the point of import_data without giving it a file?).
--
https://code.launchpad.net/~openerp-dev/openerp-web/trunk-import-kch/+merge/74216
Your team OpenERP R&D Team is subscribed to branch
lp:~openerp-dev/openerp-web/trunk-import-kch.
_______________________________________________
Mailing list: https://launchpad.net/~openerp-dev-gtk
Post to : [email protected]
Unsubscribe : https://launchpad.net/~openerp-dev-gtk
More help : https://help.launchpad.net/ListHelp
