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

Deprecate TypedMap symbols and replace with named functions #11672

Merged

Conversation

BillyAutrey
Copy link
Member

@BillyAutrey BillyAutrey commented Feb 15, 2023

Pull Request Checklist

Helpful things

Fixes

Fixes #10221

Purpose

  • Deprecates - and + Scala symbol methods in TypedMap.
  • Adds updated and removed functions that serve the same purpose.
  • Changes outside calls to +, -, $plus, and $minus to updated/removed.
  • Adds an ignore rule for MiMa compatibility - new signature doesn't break backwards compatibility.

In the future, postfix symbolic operators are going to be deprecated for methods with multiple parameters. Thus, this change is necessary to meet that need. The original ticket suggests removing all of those operators.

Background Context

I opted to deprecate the old methods, in case others are using them.

@BillyAutrey
Copy link
Member Author

Next commit will update usages of TypedMap.+() and TypedMap.-() through the code base. Just want to get feedback on this approach first.

@mkurz
Copy link
Member

mkurz commented Feb 17, 2023

So I had a look at #10199 again and the three + and - method were actually only added in that PR, which was never backported to 2.8.x. That means we would actually introduce already deprecated methods in Play 2.9 😉 That does not make sense of course so I just removed them now (in the first commit I just pushed).

The same is true for the addAttrs(varargs) method of the Play Java API, I does not yet exist in Play 2.8.x. Since it does cause some problems like

I think its best to just remove it for now to make our lifes easier. I don't think someone will miss it. Actually I was the one who introduced it for a project I was working on back then. However, it's "just" useful for the Java api, and we still keep the addAttrs(java.util.List) method, so it's still possible to pass an array via addAttrs(Arrays.asList(...))

I also updated the mima filters in the last PR.

Let's merge this and then we can rebase the Scala PR without the duplicate RequestImpl class on it.

@mkurz
Copy link
Member

mkurz commented Feb 17, 2023

btw, I was using git difftool 2.8.x -- core/play/src/main/scala/play/api/libs/typedmap/TypedMap.scala core/play/src/main/java/play/mvc/Http.java core/play/src/main/scala/play/core/j/JavaHelpers.scala how the files from the 2.8.x branch differ from the main branch/your pr.

*/
private[j] sealed trait RequestImplHelper extends JRequest {
override def addAttrs(entries: TypedEntry[_]*): JRequest = ???
}
Copy link
Member

@mkurz mkurz Feb 17, 2023

Choose a reason for hiding this comment

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

Removing addAttrs(entries: TypedEntry[_]*) allows us to get rid of this workaround for the mentioned Scala "bug" and makes life easiser for #11550

Copy link
Member

@mkurz mkurz 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 Billy!

@mkurz mkurz force-pushed the billy/10221-typemap-symbol-compat branch from 83704fd to 70e87e4 Compare February 17, 2023 23:03
@mkurz mkurz merged commit ef11817 into playframework:main Feb 17, 2023
mkurz added a commit to jtjeferreira/playframework that referenced this pull request Feb 18, 2023
mkurz added a commit to jtjeferreira/playframework that referenced this pull request Feb 27, 2023
mkurz added a commit to jtjeferreira/playframework that referenced this pull request Mar 7, 2023
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.

Multiple (non-implicit) parameters to a symbolic/infix should be avoided
2 participants