libnetplan: expose the routes list in the netdef#397
libnetplan: expose the routes list in the netdef#397daniloegea merged 1 commit intocanonical:mainfrom
Conversation
slyon
left a comment
slyon
left a comment
There was a problem hiding this comment.
Thanks, lgtm. I left one non-blocking inline comment.
What's your reasoning against the @dataclass, I think it's looking good.
| from: 192.168.0.0/24''') | ||
|
|
||
| netdef = next(netplan.netdef.NetDefinitionIterator(state, "ethernets")) | ||
| routes = [route for route in netdef.routes] |
There was a problem hiding this comment.
| routes = [route for route in netdef.routes] | |
| routes = list(netdef.routes) |
suggestion (non-blocking): I guess this way it would be easier to read and according to https://www.curs-ml.com/post/fastest-way-to-convert-an-iterator-to-a-list it would even be faster. But OTOH this is a test case, so we can keep it as-is if you want to. The same applies to ip in netdef.routes (and others) above.
There was a problem hiding this comment.
oh, yeah I think at some point I was calling str() on route and ended up leaving the list comprehension there.
|
So, I think the only thing I didn't like is passing all those objects of the same type to the constructor. One can easily get confused and put them in the wrong place. It's harder to do that using the pattern But I think the dataclass will be beneficial in other ways, such as comparing the objects for example... let's see I guess |
We could consider calling it |
2 participants
Description
I created the NetplanRoute class as a dataclass but I'm not convinced it's a good idea... I feel like it's better to assign each property separately without an
__init__method...Checklist
make checksuccessfully.make check-coverage).