python-openflow icon indicating copy to clipboard operation
python-openflow copied to clipboard

Remove '.value' behaviour

Open diraol opened this issue 9 years ago • 5 comments

We are going to simplify our library for the users by assuming that the attributes of our classes instances will always receive python basic types (int, string or list, when appropriate) or other complex objects (such as a Header object as message attributes).

Questions:

  • What to do when dealing with our "Enums" os "Bitmasks"? Consider them "complex structures (objects)"?
  • What to do with basic types such as "IPAddress" and "HWAddress"? We will accept "only" strings, or should the user add a HWAddress instance to that attribute?

diraol avatar Jan 10 '17 15:01 diraol

My 2 cents:

  • Enums and BitMasks: I prefer to treat them like a "complex structure" because the user would use it like i instead of ii:
    1. Store enum as a "complex structure":
    if myclass.myenum == MyEnum.NAME
    
    1. Store enum's value:
    if myclass.myenum == MyEnum.NAME.value
    
  • IPAddress and HWAddress: these classes were created to translate human representation to binary, so I would let them hidden from the user and accept only strings.

cemsbr avatar Jan 11 '17 15:01 cemsbr

Just some thoughts about the problem:

I believe we could have a more high-level interface in mind. There are 2 different ways to approach the issue, one that I call more 'open', and one that I call 'conservative'.

I prefer the 'conservative' one as it is less tolerant to mistakes and eases the validation process.

The 'open' one leaves too many possibilities, and makes maintenance and debugging more difficult. Some 'bad' uses like StatsRequest(body_type=1, body=body) area allowed in the 'open' or in the current approach, but would be forbidden in the 'conservative' one. 'Utility' (if we could call it that way) classes like the BitMask or the HWAdress would be hidden from the user as UBInt8 for example (see also https://github.com/kytos/python-openflow/issues/198). Bellow follows what the received parameters could be in each case:

In the 'conservative' approach:

for Enums:
    - a string, representing the attribute name of the Enum class:
        + 'OFPST_FLOW' (or simply 'FLOW')

for BitMasks:
    - a list of strings representing the attribute names:
        + ['OFPC_FLOW_STATS', 'OFPC_PORT_STATS']
          (or simply ['FLOW_STATS', 'PORT_STATS'])

for IPAdress and HWAdress:
    - a string representing the address:
        + '00:00:00:00:00:00'

In the 'open' approach (many possibilities...):

for Enums:
    - the used Enum class atribute:
        + StatsTypes.OFPST_FLOW
    - a string, representing the attribute name of the Enum class:
        + 'OFPST_FLOW'
    - the integer value of the flag:
        + 1

for BitMasks:
    - a list of the used BitMask class attributes:
        + [Capabilities.OFPC_FLOW_STATS, Capabilities.OFPC_PORT_STATS]
    - a list of strings representing the attributes:
        + ['OFPC_FLOW_STATS', 'OFPC_PORT_STATS']
    - a list of the values:
        + [1, 4]
    - an integer representing the bitmask:
        + 5

for IPAddress and HWAddress:
    - a HWAdress instance
    - a string representing the address:
        + '00:00:00:00:00:00'
    - a tuple representing the address:
        + (0, 0, 0, 0, 0, 0)

erickvermot avatar Apr 12 '17 20:04 erickvermot

My suggestion is to keep the values as the user assigned (e.g. bitmask as list of enums) so the user won't see a different value when accessing the attribute later. With this in mind, I liked:

  • a list of the used BitMask class attributes: [Capabilities.OFPC_FLOW_STATS, Capabilities.OFPC_PORT_STATS]
    • Now, we force the logic "or" operator (|) that requires more effort to convert from bitmask to options list.
  • (as already discussed before) for IPAdress and HWAdress:
    • a string representing the address: '00:00:00:00:00:00'

cemsbr avatar Apr 18 '17 23:04 cemsbr

@diraol @cemsbr @erickvermot I'm moving this to b3. We have to discuss better.

beraldoleal avatar Apr 19 '17 12:04 beraldoleal

Agreed.

diraol avatar Apr 19 '17 13:04 diraol