Fix reflection warnings

[ruroru]

No description provided.

[weavejester]

Thanks for the PR, I've made some comments on things to fix.

[weavejester]

I've been thinking about this change, and I think we should go about this in a different way:

(set! *warn-on-reflection* false)

(defn add-binary-body
  [^MultipartEntityBuilder builder key value ^ContentType mimetype
   ^String filename]
  (.addBinaryBody builder (name key) value mimetype filename))

(set! *warn-on-reflection* true)

The reason for this is that a reflection lookup should, in theory, be faster than checking the type with cond, and the code is clearer and more concise.

Setting the *warn-on-reflection* var in this way should only affect the current namespace (since it's reset on load).

[ruroru]

Haven't checked, but I doubt reflection will be faster than a conditional. Also hiding a problem behind manual handling of warn-on-reflection looks like a rather dodgy solution. I would argue that even String check can be added to conditional check, which will explicitly show supported types without trying to obfuscate them behind reflection . example could be:

(string?  value)
(.addBinaryBody builder ^String key-name ^bytes (.getBytes ^String (:value param) ^Charset default-charset)
                          ^ContentType mimetype ^String filename)

[weavejester]

I ran some benchmarks, and you're correct. I was getting 80µs for the cond, and 94µs with reflection.

I also think you're correct about the string check. In which case, perhaps we factor it out to a function:

(defn- str->bytes ^bytes [^String s]
  (.getBytes s ^Charset default-charset))

(defn- add-binary-body
  [^MultipartEntityBuilder builder key value ^ContentType mimetype
   ^String filename]
  (let [k (name key)]
    (cond
      (string? value)
      (.addBinaryBody builder k (str->bytes value) mimetype filename)
      (bytes? value)
      (.addBinaryBody builder k ^bytes value mimetype filename)
      (file? value)
      (.addBinaryBody builder k ^File value mimetype filename)
      (instance? InputStream value)
      (.addBinaryBody builder k ^InputStream value mimetype filename)
      :else
      (throw (IllegalArgumentException.
              (str "Cannot encode a value of type " (type value)
                   "as a multipart body."))))))

(defn- add-multipart-part [^MultipartEntityBuilder builder k v]
  (let [param    (if (map? v) v {:value v})
        value    (if (map? v) (:value v) v)
        mimetype (ContentType/parse
                  (or (:content-type param)
                      (when (file? value)
                        (mime/ext-mime-type (.getName ^File value)))
                      (if (string? value)
                        "text/plain; charset=UTF-8"
                        "application/octet-stream")))
        filename (or (:filename param)
                     (when (file? value) (.getName ^File value)))]
    (add-binary-body builder (name k) value mimetype filename)))

[ruroru]

I like the way it looks now. Would be a bit more happy if instead of throwing an exception, same builder would be returned, but apart from that I am happy.

[weavejester]

What's your reasoning for not throwing an exception?

[weavejester]

Looks like these lines have been accidentally indented.

[weavejester]

Looks good; merged.