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

API refactor for rlp.Serializable #61

Closed
pipermerriam opened this issue Apr 12, 2018 · 5 comments
Closed

API refactor for rlp.Serializable #61

pipermerriam opened this issue Apr 12, 2018 · 5 comments

Comments

@pipermerriam
Copy link
Member

I've some thoughts on how we might change rlp.Serializable.

Current features

  • immutable vs. mutable objects (make_mutable, make_immutable).
  • attribute access to fields
  • instantiation with either *args or **kwargs
  • cls.exclude API for creating classes with a subset of fields

Inefficiencies

  • mutability checks at runtime
  • field assignment done exclusively through __setattr__ (60% faster to use direct attribute assignment)
  • obj.fields is non-ideal since it disallows the use of any field named fields

Solution

  • use collections.namedtuple for immutable versions of the object. Fields are naturally ordered, and immutability is enforced by default.
  • use a dynamically constructed class for mutable versions of the object.
@pipermerriam
Copy link
Member Author

@carver and @gsalgado thoughts?

@carver
Copy link
Contributor

carver commented Apr 12, 2018

Seems reasonable. It would be nice to think through how we would need to re-work current subclasses, like: https://github.com/ethereum/eth-rlp/blob/master/eth_rlp/main.py#L13

@pipermerriam
Copy link
Member Author

It would be nice to think through how we would need to re-work current subclasses

The one major breaking change that I want to make is removal of obj.fields. Otherwise, I don't see anything in the linked example that would need to be reworked. I'm implementing __iter__ for use internally, so you'd even be able to remove that from the subclass.

@wjmelements
Copy link

I don't see any make_mutable. The only apparent way to make it mutable is to set _in_mutable_context to True

@carver
Copy link
Contributor

carver commented May 16, 2019

IIRC we settled on an immutable-only approach.

pacrob pushed a commit to pacrob/pyrlp that referenced this issue Sep 21, 2023
* Add breaking change type to newsfragment

* Move breaking-change -> breaking
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

No branches or pull requests

3 participants