-
Notifications
You must be signed in to change notification settings - Fork 1
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
Allow :as :text/:stream/:byte-array #6
base: master
Are you sure you want to change the base?
Allow :as :text/:stream/:byte-array #6
Conversation
- Also use :text when not explicitly given for RutledgePaulV#4 and RutledgePaulV#3
|
||
(defn coerce-stream [^InputStream stream coercion detected-charset] | ||
(case coercion | ||
:text (slurp stream :encoding detected-charset) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if we add support to muuntaja for "text/plain" that we can treat this like any other coercion-mapping.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comment #3 (comment) to continue the conversation there.
(case coercion | ||
:text (slurp stream :encoding detected-charset) | ||
:stream stream | ||
:byte-array (with-open [in stream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my experience, too often developers carelessly stuff data into byte arrays when they could've handled it in a streaming fashion instead. I guess I purposefully omitted this to force them to reckon with a stream.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RutledgePaulV , it's often about convenience. Dealing with a stream can often be a lot more cumbersome especially if the first thing you plan on doing is reading it into a byte array. Making sure things are properly consumed, opened/closed, etc can be tricky for those less experienced. IMO it's no different than trying to say "please coerce this to a clj map please" instead of asking a developer to use something like Cheshire's json streaming. I understand your point, though I don't see a reason to not provide this.
This still attempts to do auto negotiation like I believe the original spirit of this library is. This just adds the ability to force the coercion to :text, :byte-array, and :stream. If you don't specify one and the auto negotiation fails, we use :text by default then to avoid issues such as #4 and is most likely what the end user would want.
This is a breaking change if you decide to merge it, however, given the age of the project and likely usage it may be OK to just bump the version to 1.x/0.2.x or document the breaking change?
If you're OK with this MR, let me know and I'll beef up unit tests around it.