make consistent genericType initialization #427
Conversation
| @@ -114,13 +117,15 @@ class DPID(GenericType): | |||
|
|
|||
| _fmt = "!8B" | |||
|
|
|||
| def __init__(self, dpid=None): | |||
| def __init__(self, value=None): | |||
There was a problem hiding this comment.
dpid is a better variable name than value because it has more meaning. The same applies for similar modifications.
There was a problem hiding this comment.
Dpid is already the class name, there is no need to be redundant.
The same applies to the other modifications.
GenericType accepts a value parameter in its initialization. The value can be retrieved with the .value attribute. I think it makes sense to keep the same behavior for its derivatives.
Furthermore, this allows type(self)(value=x) to work as expected for any GenericType.
| @@ -237,18 +242,20 @@ class IPAddress(GenericType): | |||
| netmask = UBInt32() | |||
| max_prefix = UBInt32(32) | |||
|
|
|||
| def __init__(self, address="0.0.0.0/32"): | |||
| def __init__(self, value=None): | |||
There was a problem hiding this comment.
Thinking about the user, it might be easier to see default values in current code. In Python, only mutable objects require using None in some cases. Do you have other argument than making all the code look the same?
There was a problem hiding this comment.
I agree this is only strictly needed in cases where the init object is mutable,
but this is not about "looking the same", it is about consistency.
Almost all classes we implemented receive named parameters initially set to None in the call, making it explicit like this ensures and remembers that calling IPAddress() and IPAdress(None) will forcibly behave the same.
That said, the important part here is actually line 251 (https://github.com/kytos/python-openflow/pull/427/files#diff-83e119f254a95803fa6cdaff8bd5bee3R251), which ensures the class can be called with None, something that is not possible right now.
We could to make it accept None on line 251, and still leave the default value being set on 245, but that will only make it necessary to define the default value twice, since it'd still have to be set in line 252 for the cases where the call is made with None. So I think that would be redundant and just unnecessary.
There was a problem hiding this comment.
About consistency, you are right. The project started using None in all attributes. Later, we needed default values and decided to write it in the constructor declaration. It's not in our priorities to make this change in older code and that's why the code is how it is.
We started to set default immutable arguments together with constructor parameters because of better readability without the need to add extra lines of code. But I didn't understand why Object(None) is important. Do you have a practical example where Object(None) would be the only viable solution or a reasonably better one?
relates to #426