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

JsObject should return immutable collections (play-json 2.8/ scala 2.13) #388

Closed
987Nabil opened this issue Dec 27, 2019 · 11 comments
Closed

JsObject should return immutable collections (play-json 2.8/ scala 2.13) #388

987Nabil opened this issue Dec 27, 2019 · 11 comments

Comments

@987Nabil
Copy link

@987Nabil 987Nabil commented Dec 27, 2019

I was just migrating to scala 2.13 and realised, that the only collections related migration I had to do, was with JsObject. I was using this two fields

  /**
   * The fields of this JsObject in the order passed to the constructor
   */
  lazy val fields: collection.Seq[(String, JsValue)] = underlying.toSeq

  /**
   * The value of this JsObject as an immutable map.
   */
  lazy val value: Map[String, JsValue] = underlying match {
    case m: immutable.Map[String, JsValue] => m
    case m                                 => m.toMap
  }

What I found interesting is, that even the second comment says immutable map and underlying.toSeq is returning an immutable.Seq the return values are collection.Mapand collection.Seq. This seems odd to me. Especially because immutable collections should be considered the default, since it is what is defined in predef now (2.13).

Are there any good reasons to not change this to immutable?

@987Nabil
Copy link
Author

@987Nabil 987Nabil commented Jan 7, 2020

After checking out play json, I realized that underlying.toSeq is not returning an immutable collection.
But values for sure is always immutable and I don't see any harm in the type being more restrictive/concrete.

So my suggestion would be

  /**
   * The value of this JsObject as an immutable map.
   */
  lazy val value: immutable.Map[String, JsValue] = underlying match {
    case m: immutable.Map[String, JsValue] => m
    case m                                 => m.toMap
  }

  /**
   * Return all fields as a set
   */
  def fieldSet: immutable.Set[(String, JsValue)] = fields.toSet

@octonato
Copy link
Member

@octonato octonato commented Jan 10, 2020

This is a backwards-incompatible change. The way to go here is to add new variants that are immutable and deprecate the mutable ones.

So, ok for fieldSet. But value must stay what it is and instead we need a new variant, eg: valueMap or asMap.

valueMap aligns well with fieldSet

@octonato
Copy link
Member

@octonato octonato commented Jan 10, 2020

@14mr0n, forgot to mention. PRs are super welcome! 😉

@987Nabil
Copy link
Author

@987Nabil 987Nabil commented Jan 13, 2020

@renatocaval thanks for clarification. I'll happily do this changes and open a PR in the next days. :)

@asazernik
Copy link

@asazernik asazernik commented Jun 24, 2020

I might also jump on this when/if my workload permits.

@RichardMarto
Copy link

@RichardMarto RichardMarto commented Mar 9, 2021

Any updates on this issue? I'm able to fix it right now if needed.

@octonato
Copy link
Member

@octonato octonato commented Mar 11, 2021

@RichardMarto no updates and I don't think anyone is working on it. Feel free to pick.

Let me know if you do it so I assign to you just to signal to others that someone is already on it.

@RichardMarto
Copy link

@RichardMarto RichardMarto commented Mar 11, 2021

I'm just not sure about which version should be in since argument on @deprecated(message: String, since: String), should I use the next minor version? Where can I find more about how this project is versioned?

@RichardMarto
Copy link

@RichardMarto RichardMarto commented Mar 11, 2021

Is there any documentation that needs to be updated?
The tests that already exist should be enough for these changes, right?
Was I right to assume 2.9.3 as the version in which value will be deprecated?

@RichardMarto
Copy link

@RichardMarto RichardMarto commented Mar 11, 2021

I think I misunderstood something. fieldSet should be ignored, deprecated or just changed?
I thought that it should be changed, but it was also a backward-incompatible change.

@johnduffell
Copy link

@johnduffell johnduffell commented Jul 8, 2021

This also affects JsError and other in this file due to this import, would be great to have it fixed:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants