Skip to content
This repository has been archived by the owner on Apr 22, 2024. It is now read-only.

Remove '.value' behaviour #237

Open
diraol opened this issue Jan 10, 2017 · 5 comments
Open

Remove '.value' behaviour #237

diraol opened this issue Jan 10, 2017 · 5 comments

Comments

@diraol
Copy link
Contributor

diraol commented Jan 10, 2017

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?
@cemsbr
Copy link
Contributor

cemsbr commented Jan 11, 2017

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.

@erickvermot
Copy link
Contributor

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
#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)

@cemsbr
Copy link
Contributor

cemsbr commented Apr 18, 2017

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'

@beraldoleal beraldoleal modified the milestones: 2017.1b3, 2017.1b2 Apr 19, 2017
@beraldoleal
Copy link
Member

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

@diraol
Copy link
Contributor Author

diraol commented Apr 19, 2017 via email

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants