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

Preserve order of iterable in viaSeq #330

Merged
merged 1 commit into from
Nov 9, 2020
Merged

Preserve order of iterable in viaSeq #330

merged 1 commit into from
Nov 9, 2020

Conversation

swsnr
Copy link
Contributor

@swsnr swsnr commented May 11, 2020

Add a general property to assert this behaviour.

Fixes #329

Add a general property to assert this behaviour.

Fixes #329
@swsnr
Copy link
Contributor Author

swsnr commented May 11, 2020

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

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
Copy link
Contributor Author

swsnr commented Aug 2, 2020

A friendly ping ☺️

@swsnr
Copy link
Contributor Author

swsnr commented Oct 16, 2020

Well I guess not 🤷

@sirthias
Copy link
Member

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
Copy link
Contributor Author

swsnr commented Oct 16, 2020

No worries 🙏, 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. 😊 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 🙂

@sirthias
Copy link
Member

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
Copy link
Contributor Author

swsnr commented Oct 16, 2020

@sirthias No worries ❤️ 🤗

@Kreinoee
Copy link

@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
Copy link
Member

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

jrudolph commented Nov 9, 2020

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.

Copy link
Member

@jrudolph jrudolph left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @lunaryorn (and again sorry for the delay).

@jrudolph jrudolph merged commit 4573e0e into spray:release/1.3.x Nov 9, 2020
@swsnr
Copy link
Contributor Author

swsnr commented Nov 9, 2020

@jrudolph No worries ❤️

I would very much appreciate a 1.3.6 release with this fix, thanks 🙏 Also thanks for the work on this project ❤️

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.

DefaultJsonProtocol.viaSeq looses ordering for ordered iterables
4 participants