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

Allow :as :text/:stream/:byte-array #6

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vincentjames501
Copy link
Contributor

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.

- 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)
Copy link
Owner

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.

Copy link
Contributor Author

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
Copy link
Owner

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.

Copy link
Contributor Author

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.

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