search

spray / spray-json

A lightweight, clean and simple JSON implementation in Scala
Apache License 2.0
969 stars 190 forks source link

Preserve order of iterable in viaSeq #330

Closed swsnr closed 3 years ago

swsnr commented 4 years ago

Add a general property to assert this behaviour.

Fixes #329

swsnr commented 4 years ago

@jrudolph Would appreciate a review, and, once merged, a "soonish" release :slightly_smiling_face:

I wasn't sure what branch to base this on, so I picked the default one, i.e. release/1.3.x. Please do tell me if I ought to use another branch, and I'll update the PR accordingly.

swsnr commented 4 years ago

A friendly ping ☺️

swsnr commented 3 years ago

Well I guess not :shrug:

sirthias commented 3 years ago

Actually, this looks good to me and should do the trick! Sorry, @lunaryorn, for the non-reactivity here. The thing is: spray-json is, just like spray itself, very much in maintenance or even legacy mode. We expect noone really to use it for new projects and the existing users probably want it to remain as stable as possible. Therefore there is quite some reluctance to touch anything or change behavior, even if it might actually correct old problems.

WDYT, @jrudolph? Should there even be another release of spray-json?

swsnr commented 3 years ago

No worries :pray:, we've fixed this locally and are in the process of phasing out spray-json from our code.

But I would have appreciated if you had told me this in #329, before I opened this pull request. :blush: It's just a small change for sure, but it took time nonetheless, time I could've spent otherwise if I had known that spray won't make another release :slightly_smiling_face:

sirthias commented 3 years ago

Yes, @lunaryorn, you are right. It's definitely not nice to let you in the dark about the state of the project and not react to your tickets and PRs in a timely manner. I'd like to apologize for our negligence here!

swsnr commented 3 years ago

@sirthias No worries :heart: :hugs:

Kreinoee commented 3 years ago

@sirthias Mayby you should also make it more clear in the readme file. Currently I interpreted the text:

spray-json is largely considered feature-complete for the basic functionality it provides. It is currently maintained by the Akka team at Lightbend.

As: Fully useable for new projects, as long as it has the functionality needed for that project, as updates for removing bugs, and supportign new scala version would be made, as the project is maintained.

Based on this, we have actually used spray-json for a fair amount of new projects. Another reason we chose spray json, is that the akka http documentation, tells that spray json is the only official supported json library: https://doc.akka.io/docs/akka-http/current/common/json-support.html and we use akka-http a lot.

But if you consider spray-json to be legacy only, and new project should not pick it up, then please make that statement very clear in the readme. Mayby with and suggestion to one or more alternative projects to use instead.

sirthias commented 3 years ago

Currently I interpreted the text:

spray-json is largely considered feature-complete for the basic functionality it provides. It is currently maintained by the Akka team at Lightbend.

As: Fully useable for new projects, as long as it has the functionality needed for that project, as updates for removing bugs, and supportign new scala version would be made, as the project is maintained.

@Kreinoee AFAIK you are reading the statement in the README correctly. spray-json is still actively maintained by Lightbend. That means that it'll be pulled forward through all remaining Scala 2.x versions. Also, should any serious bugs (like a security issue) be discovered I'm sure Lightbend would be quick to plug the whole.

However, I can't comment as to their commitment to things like porting to Scala 3 for example. The Scala JSON tooling landscape is very different now from the time when spray-json was started (which was almost 10 years ago now). Meanwhile a lot of much more powerful and also faster JSON libs have popped up and aged as well (like circe). So, I'm not sure it makes sense to invest much more dev time into an oldish project like spray-json. Therefore the hesitation to add or change features outside of the "serious bug squashing" scope.

But maybe @jrudolph can comment more on Lightbend's stance toward spray-json's future...

jrudolph commented 3 years ago

Sorry about the long silence in this issue wrt any stance on this from our side. I agree with Mathias. We will most likely not add any big features but run this project in bug fixing and compatibility mode (but try to be a more responsive about issues like this one!).

To be transparent about this: We have not yet decided what to do about our recommendations in akka-http. The main reason we still show spray-json most prominently there, is that it's the most easy to support from our side. Should any serious issues arise in spray-json or the integratino with akka-http, we could fix it as needed. Alternatively, we could recommend any other third-party library (most of which are supported through https://github.com/hseeberger/akka-http-json) but that would leave us in the more difficult spot to "bless" a third-party library users should go with and also provide some level of support for a json library where we cannot guarantee that changes are done when needed. So, we will have to think some more about it.

All that said, I'm now going to the open PRs and issues and see which issues should be fixed and release a 1.3.6 with those most important fixes. Apologies again to neglect issues like this one for quite some time.

swsnr commented 3 years ago

@jrudolph No worries :heart:

I would very much appreciate a 1.3.6 release with this fix, thanks :pray: Also thanks for the work on this project :heart: