Skip to content
This repository was archived by the owner on Sep 11, 2020. It is now read-only.

Fix pktline api #89

Merged
merged 5 commits into from
Oct 19, 2016
Merged

Fix pktline api #89

merged 5 commits into from
Oct 19, 2016

Conversation

alcortesm
Copy link
Contributor

@alcortesm alcortesm commented Oct 19, 2016

The old pktline API only had one method: New, that receives the
payloads and return the new PktLine, that was an io.Reader. This means
you have to prepare the contents beforehand, in a [][]byte or a []string and then
call the ctor to build the pktlines:

payloads := []string{
    "hello\n",
    "world!\n",
    "",
}

p, _ := pktline.NewFromStrings(payloads...)

io.Copy(os.Stdout, p)

Now, the construction of the pktlines and the methods to add payloads and flush-pkts are
separated and the reader is an attribute:

p := pktlines.New()

_ = p.AddString(
    "hello\n",
    "world!\n",
)
p.AddFlush()

io.Copy(os.Stdout, p.R)

I have also change the name of the package to its plural version: pktlines, as the package purpose is to handle several pktlines together, not just one.

The old pktline API only had one method: New, that receives the
payloads and return the new PktLine, that was an io.Reader.  This means
you have to prepare the contents beforehand, in a [][]byte and then
call the ctor to build the pktlines.

Now, the construction of the pktlines and the method to add payloads are
separated:

  New() // creates an empty PktLines
  AddFlush()
  Add(pp ...[]byte)
  AddString(pp ...string)

and a PktLines has a public member R, which is the io.Reader of the
pktlines added.
@mcuadros
Copy link
Contributor

IMHO the structure can be Packfiles, but the name of the package should be the name of the format.

@alcortesm
Copy link
Contributor Author

Agree @mcuadros, will do that.

@alcortesm
Copy link
Contributor Author

I will also change the public attribute to a Reader method.

Copy link
Contributor

@dpordomingo dpordomingo left a comment

Choose a reason for hiding this comment

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

LGTM with a little suggestion

// New returns an empty PktLines (with no payloads) ready to be used.
func New() *PktLines {
return &PktLines{
R: io.MultiReader([]io.Reader{}...),
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe better if it would be something like p.Reader()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, will do!

@alcortesm
Copy link
Contributor Author

alcortesm commented Oct 19, 2016

Package name changed to pktline (singular).

R attribute removed, now PktLines are Readers themselves.

PTAL.

Copy link
Contributor

@dpordomingo dpordomingo left a comment

Choose a reason for hiding this comment

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

LGTM

@mcuadros mcuadros merged commit d0ea76c into src-d:master Oct 19, 2016
@alcortesm alcortesm deleted the fix-pktline-api branch October 20, 2016 11:07
mcuadros pushed a commit that referenced this pull request Jan 31, 2017
* Change pktline API so you can add payloads

The old pktline API only had one method: New, that receives the
payloads and return the new PktLine, that was an io.Reader.  This means
you have to prepare the contents beforehand, in a [][]byte and then
call the ctor to build the pktlines.

Now, the construction of the pktlines and the method to add payloads are
separated:

  New() // creates an empty PktLines
  AddFlush()
  Add(pp ...[]byte)
  AddString(pp ...string)

and a PktLines has a public member R, which is the io.Reader of the
pktlines added.

* metalinter

* change package name from pktlines to pktline

* change package name from pktlines to pktline for true

* make pktlines a reader instead of have a reader
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants