Skip to content

Clean up API #5

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Mar 7, 2019
Merged

Clean up API #5

merged 2 commits into from
Mar 7, 2019

Conversation

@maxhawkins
Copy link
Contributor

  • Rename Reader -> Decoder
  • Return a parsed packet from the decoder
  • Return only a parsed packet (no header) from Unmarshal

Am I missing anything? I want to make the API more consistent and easy to use for the v2 release.

@maxhawkins
Don't return a Header in Unmarshal
99de561 
The header is now embedded in the returned packet.

Relates to #4
@maxhawkins
Rename Reader -> Decoder, return a Packet
dfcb12b 
Reader implies Read([]byte) (int, error) signature, so another
name is better. Decoder works because it's similar to json.Decoder
from the stdlib.

Now that we have a Packet type we can return the parsed packet and
use the included header.

Relates to #4
@maxhawkins maxhawkins requested a review from wdouglass February 25, 2019 03:25
@backkem backkem added the review label Feb 25, 2019
@maxhawkins
Copy link
Contributor Author

@kixelated I know you had some opinions about Reader too

wdouglass
wdouglass approved these changes Mar 7, 2019
View reviewed changes
Copy link
Member

@wdouglass wdouglass left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that changing "Read" to "Decode" is a bit of a sideways change, but I like the idea of returning a packet a lot.

Sorry this review took so long, i've been super busy.

@maxhawkins
Copy link
Contributor Author

Thanks for the review.

The idea behind doing Decode rather than Read is that Read doesn't match the stdlib's io.Reader interface so some may be confused. Decoder is like encoding/json's Encoder/Decoder and matches the Marshal/Unmarshal methods.

@wdouglass
Copy link
Member

wdouglass commented Mar 7, 2019 •
edited
Loading

Ahh, that makes a ton of sense. Carry on and merge!

@maxhawkins maxhawkins merged commit a88966a into master Mar 7, 2019
@backkem backkem removed the review label Mar 7, 2019
@mjmac mjmac mentioned this pull request Mar 7, 2019
Closed
@maxhawkins maxhawkins deleted the api branch April 4, 2019 22:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wdouglass wdouglass wdouglass approved these changes

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@maxhawkins @wdouglass @backkem