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

Add a package-level doc string for buffered and example program #1639

Merged
merged 2 commits into from
Mar 9, 2017

Conversation

aturley
Copy link
Member

@aturley aturley commented Mar 6, 2017

This commit adds a package-level doc string for the buffered package
in a few file called buffered.pony. The doc string contains a short
example program.

let b = recover iso Array[U8] end

for x in writer.done().values() do
b.append(x)
Copy link
Member

@jemc jemc Mar 6, 2017

Choose a reason for hiding this comment

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

Could we do reader.append(x) here instead?

Also, maybe use chunk is a more descriptive name than x? Since this is a form of documentation, I think more descriptive is better.

Copy link
Member Author

Choose a reason for hiding this comment

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

I actually wanted to do reader.append(x), but it doesn't quite work because the append takes an Array[U8] val but x is a ByteSeq. I had something like this earlier:

for x in writer.done.values() do
  match x
  | let s: String => reader.append(x.array())
  | let s: Array[U8] val => reader.append(x)
  end
end

But that felt not great.

If there's a better way to do this that I'm not seeing, please let me know.

I can make the change to chunk, I agree that it is much more descriptive.

Copy link
Member

Choose a reason for hiding this comment

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

Eek! That lack of symmetry is rather irksome, isn't it?

I'd argue that we need to fix Reader.append then to accept a ByteSeq. A quick excursion through the code suggests that it can be done without breaking any user code, with the following diff:

diff --git a/packages/buffered/reader.pony b/packages/buffered/reader.pony
index f3f37b5..e5c91bd 100644
--- a/packages/buffered/reader.pony
+++ b/packages/buffered/reader.pony
@@ -61,9 +61,9 @@ class Reader
   """
-  embed _chunks: List[(Array[U8] val, USize)] = _chunks.create()
+  embed _chunks: List[(ByteSeq, USize)] = _chunks.create()
   var _available: USize = 0
-  var _search_node: (ListNode[(Array[U8] val, USize)] | None) = None
+  var _search_node: (ListNode[(ByteSeq, USize)] | None) = None
   var _search_len: USize = 0
 
   fun size(): USize =>
@@ -79,7 +79,7 @@ class Reader
     _chunks.clear()
     _available = 0
 
-  fun ref append(data: Array[U8] val) =>
+  fun ref append(data: ByteSeq) =>
     """
     Add a chunk of data.
     """
@@ -547,13 +547,13 @@ class Reader
     end
 
     var node = if _search_len > 0 then
-      let prev = _search_node as ListNode[(Array[U8] val, USize)]
+      let prev = _search_node as ListNode[(ByteSeq, USize)]
 
       if not prev.has_next() then
         error
       end
 
-      prev.next() as ListNode[(Array[U8] val, USize)]
+      prev.next() as ListNode[(ByteSeq, USize)]
     else
       _chunks.head()
     end
@@ -574,7 +574,7 @@ class Reader
         break
       end
 
-      node = node.next() as ListNode[(Array[U8] val, USize)]
+      node = node.next() as ListNode[(ByteSeq, USize)]
     end
 
     _search_node = node

This commit adds a package-level doc string for the `buffered` package
in a few file called `buffered.pony`. The doc string contains a short
example program.
@aturley aturley force-pushed the enhancement/buffered-doc-string branch from 0281ef7 to 54e5f9b Compare March 7, 2017 14:50
@jemc
Copy link
Member

jemc commented Mar 7, 2017

As I said in the code comment, I think we need to fix Reader.append to take a ByteSeq. Doing so would break no user code, and it would resolve that unexpected asymmetry between Reader and Writer, so I'd definitely call it an easy win "principle of least surprise bugfix".

I can file a PR to do this later if no one beats me to it.

In the meantime, I will merge this docstring, and it can be amended when the API is fixed.

Thanks!

env.out.print(reader.u32_be().string()) // prints 42
env.out.print(reader.f32_be().string()) // prints 3.14
end
``
Copy link
Member

Choose a reason for hiding this comment

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

Whoops!

I think this line is now missing the third backtick, required to close the code block.

@jemc
Copy link
Member

jemc commented Mar 8, 2017

@aturley, even though this hasn't been merged yet due to the missing backtick, I went ahead and filed #1644.

Assuming that PR isn't controversial, can you go ahead and amend the example program to match, using reader.append(chunk) as we discussed?

There was a typo in the package documentation for `Buffered`. A back-tick was missing from the code sample.
@aturley
Copy link
Member Author

aturley commented Mar 8, 2017

@jemc let's wait and see how #1644 shakes out before merging. If we figure out a way to make the change to the API then I'll update this documentation to reflect that. If we decide to leave the API unchanged then we can merge this.

@jemc jemc merged commit 760b351 into master Mar 9, 2017
@jemc jemc deleted the enhancement/buffered-doc-string branch March 9, 2017 17:15
@jemc
Copy link
Member

jemc commented Mar 9, 2017

As we discussed offline, decided to merge this, then rebase #1644 so it can update the docstring if it gets merged.

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

Successfully merging this pull request may close these issues.

2 participants