Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 31 commits into from
Feb 19, 2023

Conversation

ndokter
Copy link
Owner

@ndokter ndokter commented Jan 14, 2023

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

@lowdef
Copy link
Contributor

lowdef commented Feb 12, 2023

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

@lowdef
Copy link
Contributor

lowdef commented Feb 13, 2023

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
Copy link
Owner Author

ndokter commented Feb 14, 2023

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

@ndokter ndokter mentioned this pull request Feb 14, 2023
@lowdef
Copy link
Contributor

lowdef commented Feb 15, 2023

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
Copy link
Owner Author

ndokter commented Feb 15, 2023

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:

  • _item_names was added back in favor of _telegram_data, which i could not get rid of right away due to issues with the fluvius telegram and conflicting keys.
  • to_json added to the MbusDevice and integrated into Telegram.to_json
  • __str__ implemented on MbusDevice and integrated into Telegram.__str__

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

@ndokter ndokter merged commit de167c8 into master Feb 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants