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

Rename BytesN to ByteVector, rename Bytes to ByteList #1480

Merged
merged 1 commit into from
Nov 18, 2019
Merged

Conversation

protolambda
Copy link
Collaborator

@protolambda protolambda commented Nov 15, 2019

People reading the pyspec SSZ util code got confused by Bytes vs BytesN. So this PR changes it to a more verbose but also more clear ByteList and ByteVector. No functional changes.

Fixes #1479

@protolambda protolambda added scope:SSZ Simple Serialize general:presentation Presentation (as opposed to content) labels Nov 15, 2019
@djrtwo
Copy link
Contributor

djrtwo commented Nov 16, 2019

I support this

@JustinDrake
Copy link
Collaborator

ByteList, ByteVector, Bitlist, Bitvector

I also support this but I guess we need consistent capitalisation. Could rename Bitlist to BitList.

@protolambda
Copy link
Collaborator Author

Agree with Justin, but there's something to mixed capitalisation too: ByteVector[N] is exactly a Vector[Byte, N], whereas Bitvector is not a Vector[Bit, N]: it's like a bitfield instead, a full word without stops. Same thing for the list variant.

@djrtwo
Copy link
Contributor

djrtwo commented Nov 18, 2019

Ah, you're right @protolambda. Unfortunately the subtlety in the difference in capitalization doesn't really send much of a signal to a dev imo.

That said, I think I lean toward keeping as is to reduce overhead in changing implementations and capturing the minor difference

@djrtwo djrtwo merged commit 6ef79ac into dev Nov 18, 2019
@djrtwo
Copy link
Contributor

djrtwo commented Nov 18, 2019

Ah, maybe should have moved to v09x. I suppose no need to get this out into a release soon

@protolambda protolambda deleted the ssz-bytes-naming branch February 9, 2020 00:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
general:presentation Presentation (as opposed to content) scope:SSZ Simple Serialize
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BytesN SSZ Alias is Misleading
3 participants