hmil / RosHTTP

Unified Scala.js + Scala HTTP client API
MIT License
126 stars 24 forks source link

JSArray #65

Closed antonkulaga closed 6 years ago

antonkulaga commented 7 years ago

I added JS arrays to make it possible to give them as part of JSONobjects in POST requests

antonkulaga commented 7 years ago

Note: based on top of https://github.com/hmil/RosHTTP/pull/64

hmil commented 7 years ago

Thank you for your contribution. I apologize for the late reply, I somehow don't get email notifications for PRs on this repo...

Could you rebase on top of master and then change the following?

class JSONArray(values: JSONValue*) extends JSONValue {
   override def toString: String = "[" + values.mkString(",") + "]"
}

I'd change the constructor to take an actual Seq of JSONValues rather than a vararg and add a companion object with an apply method to do the syntactic sugar. Something like:

object JSONArray {
  def apply(values: JSONValue*): JSONArray = new JSONArray(Seq(values: _*))
}

This way JSONArray follows the same design as JSONObject which keeps the API consistent.

Also if not adding a unit test case, could you at least add a usage example in README.md and run test/update-readme-sanity-check.sh so that this feature is tracked in CI.

Thanks

Maurycy-Sokolowski commented 6 years ago

What is the status of this?

hmil commented 6 years ago

Got no news from @antonkulaga since my last message. As always, this repo is OSS and can only move as fast as the community is willing to make it move. In addition, the in-browser tests are currently broken so I'm reluctant to merging #70 and #72 even though I think they are ready. If someone is willing to finish the job started on this PR I might just end up merging everything despite the broken tests and publish a snapshot release. Contact me if you need help or need something merged but other than that I'm currently off to other projects.

Maurycy-Sokolowski commented 6 years ago

I resolved it for now just adding:

class JSONArray(values: JSONValue*) extends JSONValue { override def toString: String = "[" + values.mkString(",") + "]" }

object JSONArray { def apply(values: JSONValue): JSONArray = new JSONArray(values: _) }

import JSONArray._

anywhere needed.