Support printing to arbitrary Writers #537

Merged
merged 3 commits into from Jan 9, 2017

Conversation

Projects
None yet
3 participants
@travisbrown
Member

travisbrown commented Jan 7, 2017

Addresses #536. In addition to the new prettyBytes I've made a few other small changes that simplify things a bit and seem to give us a few percent more throughput in the benchmarks.

I'm not currently exposing the ability for users to print to their own Appendables. I'm not totally opposed to the idea, but I'd prefer not to add methods that are fundamentally about mutation unless there's a really clear need.

@travisbrown travisbrown referenced this pull request in finagle/finch Jan 7, 2017

Closed

String-less encoding #676

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Jan 7, 2017

Current coverage is 82.67% (diff: 100%)

Merging #537 into master will increase coverage by 0.45%

@@             master       #537   diff @@
==========================================
  Files            70         70          
  Lines          1974       2014    +40   
  Methods        1828       1874    +46   
  Messages          0          0          
  Branches        146        140     -6   
==========================================
+ Hits           1623       1665    +42   
+ Misses          351        349     -2   
  Partials          0          0          

Powered by Codecov. Last update a8a5268...b704b89

codecov-io commented Jan 7, 2017

Current coverage is 82.67% (diff: 100%)

Merging #537 into master will increase coverage by 0.45%

@@             master       #537   diff @@
==========================================
  Files            70         70          
  Lines          1974       2014    +40   
  Methods        1828       1874    +46   
  Messages          0          0          
  Branches        146        140     -6   
==========================================
+ Hits           1623       1665    +42   
+ Misses          351        349     -2   
  Partials          0          0          

Powered by Codecov. Last update a8a5268...b704b89

+ printJsonAtDepth(writer)(json, 0)
+
+ writer.close()
+ bytes.toByteArray

This comment has been minimized.

@vkostyukov

vkostyukov Jan 8, 2017

Collaborator

I wish we could avoid copying here. Even though there is no API to retrieve the underlying buffer directly, people seem to use this simple trick to do that. Please, note that this means we'd need to change the return type to ByteBuffer since the underlying Array[Byte] may contain more bytes than we need so we have to crop it.

I think we can also expose that as a standalone method (printByteBuffer) if we're worried about this extra complexity. What do you think @travisbrown?

@vkostyukov

vkostyukov Jan 8, 2017

Collaborator

I wish we could avoid copying here. Even though there is no API to retrieve the underlying buffer directly, people seem to use this simple trick to do that. Please, note that this means we'd need to change the return type to ByteBuffer since the underlying Array[Byte] may contain more bytes than we need so we have to crop it.

I think we can also expose that as a standalone method (printByteBuffer) if we're worried about this extra complexity. What do you think @travisbrown?

@travisbrown

This comment has been minimized.

Show comment
Hide comment
@travisbrown

travisbrown Jan 8, 2017

Member

Ah, good point, @vkostyukov. I've just pushed an update—what do you think?

Member

travisbrown commented Jan 8, 2017

Ah, good point, @vkostyukov. I've just pushed an update—what do you think?

@vkostyukov

This comment has been minimized.

Show comment
Hide comment
@vkostyukov

vkostyukov Jan 9, 2017

Collaborator

Looks great! I really like how simple EnhancedByteArrayOutputStream could be.

Collaborator

vkostyukov commented Jan 9, 2017

Looks great! I really like how simple EnhancedByteArrayOutputStream could be.

@travisbrown travisbrown merged commit 519038b into master Jan 9, 2017

4 checks passed

codecov/patch 100% of diff hit (target 82.21%)
Details
codecov/project 82.67% (+0.45%) compared to a8a5268
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@travisbrown

This comment has been minimized.

Show comment
Hide comment
@travisbrown

travisbrown Jan 9, 2017

Member

Ugh, was just running Scala.js tests for the 0.7.0-M2 release and get this:

[error] Referring to non-existent method java.io.BufferedWriter.<init>(java.io.Writer)
[error]   called from io.circe.Printer.prettyByteBuffer(io.circe.Json)java.nio.ByteBuffer

I guess we'll have to make this a JVM-only method—I'll get started on that asap.

Member

travisbrown commented Jan 9, 2017

Ugh, was just running Scala.js tests for the 0.7.0-M2 release and get this:

[error] Referring to non-existent method java.io.BufferedWriter.<init>(java.io.Writer)
[error]   called from io.circe.Printer.prettyByteBuffer(io.circe.Json)java.nio.ByteBuffer

I guess we'll have to make this a JVM-only method—I'll get started on that asap.

@travisbrown travisbrown deleted the topic/writer-printer branch Apr 20, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment