Code review comment for lp:~sylvain-pineau/checkbox/bug1091633

Revision history for this message
Zygmunt Krynicki (zyga) wrote :

While I don't find anything wrong with it just by looking at it I have a few general comments.

1) There are no comments in the code.

It's really hard to justify if something is correct or not. There is no rationale
explained in the code. I could look at the whole code, the history, etc and try to guess why something's
done the way it is but it's fragile and I'd rather have a code that is self explanatory to some degree.

2) There are layering issues in the coee.

For me something that's called parsers/udevadm should _not_ have any logic in it.
I'm talking about stuff like

14 + if self.bus == "sound":
15 + return "AUDIO"

This is _certainly_ something you want at a separate, higher layer. Otherwise the whole thing is ill defined
and harder to test. I'd like to see a pure parser for udevadm data along with a data layer that just
represents that and a separate mapping layer where we get our magic "WIRELESS" devices and stuff like that.

3) There are no tests being added that verify stuff works.

Writing tests is hard when things are untested and not designed to be tested easily but we just have
to get better at it. Especially for stuff like that, which is causing problems.

While I won't veto landing it we should strive to be better at those things.

« Back to merge proposal