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

Refactor decode implementation in vlen codecs #135

Open
alimanfoo opened this issue Nov 29, 2018 · 4 comments
Open

Refactor decode implementation in vlen codecs #135

alimanfoo opened this issue Nov 29, 2018 · 4 comments
Milestone

Comments

@alimanfoo
Copy link
Member

As noticed in #81, there is a lot of code duplication in the implementation of the decode() methods in the vlen codecs. Would be good to refactor and remove code duplication.

@jeromekelleher
Copy link
Member

I had a quick crack at this, and it mostly works (we just need a def _decode_item(self, data, size) method defined in sub classes). However, there's some Cython trickiness involved and this didn't work as expected for the Array version. I don't know enough about Cython to dig into it I'm afraid, and I really must stop procrastinating from what I'm actually supposed to be doing!

@alimanfoo
Copy link
Member Author

I had a quick crack at this, and it mostly works (we just need a def _decode_item(self, data, size) method defined in sub classes). However, there's some Cython trickiness involved and this didn't work as expected for the Array version. I don't know enough about Cython to dig into it I'm afraid, and I really must stop procrastinating from what I'm actually supposed to be doing!

Yeah, don't you have a proposal deadline 😃

Would be cool if you could whack whatever you have in a PR, just to take a look.

@jeromekelleher
Copy link
Member

Ack, I nuked it all. It's very simple though, just adding a VlenBase class, putting the decode method in there and then having an implementation of def _decode_item(self, data, size) in each subclass. Worked fine for the non-array subclasses.

@alimanfoo
Copy link
Member Author

😄 no worries, thanks for the info.

@alimanfoo alimanfoo added this to the v0.7.0 milestone Nov 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants