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

Unpack should return an object #130

Open
cemsbr opened this issue Jul 25, 2016 · 8 comments
Open

Unpack should return an object #130

cemsbr opened this issue Jul 25, 2016 · 8 comments

Comments

@cemsbr
Copy link
Contributor

cemsbr commented Jul 25, 2016

Refactor unpack methods to return the unpacked object instead of being an inplace method.

Probably the TODO creator meant implementing a class method so we would call FixedTypeList.unpack(buff), for instance.

@diraol
Copy link
Contributor

diraol commented Apr 25, 2017

@beraldoleal I don't think we will be able to reach this feature before #198 and #237.

@diraol diraol removed their assignment Apr 25, 2017
@diraol diraol modified the milestones: 2017.1b3, 2017.1b2 Apr 27, 2017
@beraldoleal
Copy link
Member

@diraol ok, no problem. b3 is a good target.

@diraol diraol removed this from the 2017.1b3 milestone Jun 12, 2017
@diraol diraol added this to the 2017.2 milestone Jun 20, 2017
@cemsbr
Copy link
Contributor Author

cemsbr commented Jul 25, 2017

I was thinking about adding a class method from_bytes like Python int.from_bytes to be more pythonic. What do you think?

Edit: Some unpack methods rely on instance attributes (e.g. enum_ref). Thus, a class method won't work for all cases.

@erickvermot
Copy link
Contributor

erickvermot commented Jul 26, 2017

@cemsbr Why not use python's method directly to unpack instead of using struct, keeping '.value', etc.. that's what I proposed here: #198 (comment)
master...erickvermot:value_test
(line 19 - master...erickvermot:value_test#diff-83e119f254a95803fa6cdaff8bd5bee3R19)

@cemsbr
Copy link
Contributor Author

cemsbr commented Jul 27, 2017

@erickvermot, I think the pattern pack and _unpack, get_size and _get_size, etc is not the best OOP solution.

@erickvermot
Copy link
Contributor

@cemsbr why not?

@cemsbr
Copy link
Contributor Author

cemsbr commented Jul 27, 2017

There's no need to create extra private methods. We can have only the public ones and children can override them if necessary.

@erickvermot
Copy link
Contributor

erickvermot commented Jul 28, 2017

@cemsbr Yes, you can, and this is the way it is done now. But this makes the derivative classes have to rewrite the docstring, and any other processing that is done prior to the actual packing action (like treating the optional arguments, or updating the length field in GenericMessages derivatives), which leads to massive amounts of redundant code, like in the current implementation.
a4c1f05 commit alone for example leads to a reduction of about 200 lines of code. If we decide not to accept optional value argument the reduction is even greater.
The way I propose, (if needed) the user only needs to reimplement the actual packing action, and the user will remain calling pack in the normal way and will always have access to the docstring and other preprocessing without the need to worry about them.

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