search

ndokter / dsmr_parser

Library to parse Dutch Smart Meter Requirements (DSMR) telegrams.
MIT License
110 stars 64 forks source link

issue-51-telegram refactored TelegramParser.parse to return Telegram … #121

Closed ndokter closed 1 year ago

ndokter commented 1 year ago

Refactored TelegramParser.parse to return Telegram object. Telegram object now does not do parsing anymore.

lowdef commented 1 year ago

we need a __str__ and to_json() still on the MBusDevice

lowdef commented 1 year ago

hi Nigel,

why do you rewrite __iter__ to a more complicated indirect variant? Creating the item_name list during creation and iterating over that is more straightforward. In fact the python class is already a dictionary in itself. I would go even so far as to drop the _telegram_data dict completely as it is no longer needed when all the data is contained in the class attributes.

The same pattern can then be applied to the MBusDevice, as it also needs to be printable, iterable and jsonizable.

Best, Hans Erik

ndokter commented 1 year ago

I've mainly refactored it because i removed _item_names which wasn't in use anymore and therefor broke the __iter__ output.

I'm not sure if i like using the class attributes themselves for everything. It will probably by default expose all attributes of the class even the 'private' ones. I could maybe work around that for _mbus_devices, but i'd rather just have more visibility and control over it. It will also cause small challenges with implementing the len and getitem methods. I'd rather focus on getting the functionality done and maybe check out these internals later on

lowdef commented 1 year ago

No, The idea was to only expose the object attributes that are listed in item_names, by iterating over them, so no exposure of 'private' attributes will take place. The item_names only contains the attribute names that we want to expose, that is why we explicitly add them there during the parsing stage. (Actually during analysis for #122 it took me a long time to find out what was going on here and in the end I had to reintroduce it in an altered manner. Adding the book keeping to the add()method.)

What is the reason for using the obis_reference in get_item? In my view the whole point of the telegram object was to abstract away from obis_references and all that transport format cumbersomeness. At least that was the reason the Telegram object was introduced.

ndokter commented 1 year ago

I added getitem to have some form of backwards compatibility. I could remove it, but that would mean the refactor gets even bigger by having to update all unit tests as well.

Made some progress again:

Edit: we could decide to fully remove all the key logic and only use attribute based method. Then release it as 2.0.0 (which we probably should anyway due to backwards compatibility issues)

Edit2: made the Telegram fully compatible with the current dictionary. Replaced Telegram.get_mbus_devices() with Telegram.MBUS_DEVICES to be a bit more consistent