Code review comment for ~raharper/curtin:feature/block-discover

Revision history for this message
Dan Watkins (oddbloke) wrote :

Checkpointing my review before I dig in to the large block of added code line-by-line, and also to kick off a higher-level discussion so we can have that in parallel with my more detailed review:

In that large block of added code, it looks like the parse_* functions are using the same pattern (i.e. parse_foo calls foo_asdict, which then has some helpers). I wonder if we could reduce repeated code and make it a little more formally clear which functions are related to one another if we encapsulated this in a class structure. I'm thinking something like (using raid as the concrete example):

class ProbertParser:

    def parse(self, probe_data):
        configs = []
        errors = []
        for devname, data in probe_data.get(self.probe_data_key, {}).items():
            entry = self.asdict(data, probe_data)
            if entry:
                try:
                    validate_config(entry)
                except ValueError as e:
                    errors.append(e)
                    continue
                configs.append(entry)
        return (configs, errors)

class RaidProbertParser(ProbertParser):

    probe_data_key = 'raid'

    def asdict(self, data, probe_data):
        <the current raid_asdict code>

This pattern wouldn't work perfectly for every parse_* function, but the advantage of using classes is that only those objects that require something extraordinary from the parse method will redefine it (whereas currently you have to keep all the parse_* definitions in your head to effectively determine which ones are using the standard pattern and which are doing something different).

If it turns out that there isn't enough commonality between the parse_* functions to justify a method shared on the super-class, I still think class definitions would be a good way of encapsulating the functions that are required for a particular parse_* function, which would make the code easier to grok.

(
```
$ python -m this | tail -n1
Namespaces are one honking great idea -- let's do more of those!
```
;-)

« Back to merge proposal