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

Explicitly set byte order in new #8

Closed
sturfee-petrl opened this issue Sep 15, 2017 · 5 comments
Closed

Explicitly set byte order in new #8

sturfee-petrl opened this issue Sep 15, 2017 · 5 comments

Comments

@sturfee-petrl
Copy link

sturfee-petrl commented Sep 15, 2017

I spent all day to find this problem 😞. It was absolutely not obvious.
f1b53c4#commitcomment-24315680

Byte order is only one important option when you need work with binary. There are sense to add it directly to signature.

I propose explicitly add byte order to new functions. It's go way. It should be obvious.

func NewPacker(writer io.Writer, byteOrder binary.ByteOrder) *Packer{}

func NewUnpacker(reader io.Reader, byteOrder binary.ByteOrder) *Unpacker{}

I thought about NewPackerWithOrder but it's not good idea. binpacker single purpose package and it should be concise. SetByteOrder would be unused.

⚠️Comparability is broken. ⚠️ At least people will see immediately compilation error.

@im7mortal
Copy link
Collaborator

im7mortal commented Sep 15, 2017

Compatibility instead of comparability 😁

@zhuangsirui
Copy link
Owner

Thanks for your advise. But I think use Semantic Versioning to handle this situation is a better idea.

@zhuangsirui
Copy link
Owner

@sturfee-petrl @im7mortal So sorry for this issue. I will use Semantic Versioning later.

@im7mortal
Copy link
Collaborator

im7mortal commented Sep 22, 2017

I want to reassure you one more time.

There are only 2 options LittleEndian or BigEndian.
It's much more clear to set it in New then do redundant SetByteOrder.
There are no more important parameters for the binpacker .

Like I told before 👉 go way is when code is ridiculously simple 👈. There no any clearance why it have LittleEndian by default. It's black box. 💥

There a lot of examples when New function have important explicit parameters in standard library.
For example encoding/base32

    func NewDecoder(enc *Encoding, r io.Reader) io.Reader
    func NewEncoder(enc *Encoding, w io.Writer) io.WriteCloser

⚠️ You can notice that Encoding even first parameter! ⚠️ Because it's important to tell people what they are going to do!! ❗

LittleEndian and BigEndian are also encoding
People should first choose encoding. Nobody forgot about Reader or Writer. But everyone forgot about SetByteOrder. 💯

@zhuangsirui
Copy link
Owner

@im7mortal I'm think about this again and I think your idea is the correct solution. I'm a little busy these days, can you send a PR for this please?

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