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

Rename Ning into Ahc #5224

Merged
merged 1 commit into from Nov 30, 2015
Merged

Rename Ning into Ahc #5224

merged 1 commit into from Nov 30, 2015

Conversation

slandelle
Copy link
Contributor

Please stop advertising AsyncHttpClient as Ning.

The Ning company hasn't sponsored AsyncHttpClient for many years, and there's almost no single line of code left in there from back then.
It's just like someone was advertising Play! as Zenexity.

Now that Play 2.5 will be using AHC 2 with a new package and maven groupId that no longer mention Ning, it would be great if you could finally use a proper name.

Thanks!

@lightbend-cla-validator
Copy link

Hi @slandelle,

Thank you for your contribution! We really value the time you've taken to put this together.

Before we proceed with reviewing this pull request, please sign the Typesafe Contributors License Agreement:

http://www.typesafe.com/contribute/cla

@richdougherty
Copy link
Member

@slandelle we're happy to change the class names to reflect the work of the AsyncHttpClient project. You do great work and we're happy to support you.

The changes in the PR look fine to me, but one thing I'd like to see is an easier migration path for our users. When we rename classes we try to keep the old classes around for at least one release. We do the same for config. If we make the change in Play 2.5 then we can remove the old Ning classes and config in Play 2.6 once users have had a chance to migrate their code.

A good rule of thumb is that all the config and classes mentioned in our Play 2.4 Scala and Java WS docs should still be usable in Play 2.5.

For classes, probably the easiest thing to do is to create some lightweight Ning* classes that extend the new Ahc* classes. They can be thin wrappers around the AHC functionality and marked @Deprecated.

For config you can use the PlayConfig.getDeprecated method. Using the getDeprecated method, the config code in AhcWSClientConfigParser would look something like this:

def parse(): NingWSClientConfig = {
    // Helper to read from play.ws.ahc, falling back to play.ws.ning
    // with a deprecation warning.
    def get[A: ConfigLoader](name: String): A = {
      PlayConfig(configuration).getDeprecated[A](s"play.ws.ahc.$name", s"play.ws.ning.$name")
    }
    val allowPoolingConnection = get[Boolean]("allowPoolingConnection")
    val allowSslConnectionPool = get[Boolean]("allowSslConnectionPool")
    val ioThreadMultiplier = get[Int]("ioThreadMultiplier")
    ...
}

@gmethvin
Copy link
Member

gmethvin commented Nov 9, 2015

I agree with Rich re the migration changes. I would also suggest renaming NingAsyncHttpClientConfigBuilder to simply AhcConfigBuilder. That long class name has always bothered me and replacing Ning with Ahc just makes it redundant now.

@slandelle
Copy link
Contributor Author

@richdougherty My next question is: will there be a Play 2.6 w/ AHC? At some point, I expect AHC to be replaced by Akka HTTP.

@slandelle
Copy link
Contributor Author

The only classes referenced in the documentation I could find are:

  • NingWSClientConfig
  • NingAsyncHttpClientConfigBuilder
  • NingWSClient

Does this mean that other undocumented classes such as NingWSClientConfigFactory, NingWSClientConfigParser, NingWSComponents, WSClientProvider and NingWSModule can go away?

@richdougherty
Copy link
Member

@slandelle Here's a list of classes I think we should remove and classes we should deprecate. The list is basically the same as your list except I added NingWSClientConfigFactory because it is referenced in the Java docs and I also added NingWSComponents because it is probably used by some advanced users even though it isn't documented!

Java
  • play.libs.ws.ning.NingWSAPI - remove
  • play.libs.ws.ning.NingWSModule - remove
  • play.libs.ws.ning.NingWSClient - deprecate, extend AHC class
  • play.libs.ws.ning.NingWSCookie - remove
  • play.libs.ws.ning.NingWSRequest - remove
  • play.libs.ws.ning.NingWSResponse - remove
Scala

NingConfig.scala

  • play.api.libs.ws.ning.NingWSClientConfig - deprecate, extend AHC class
  • play.api.libs.ws.ning.NingWSClientConfigFactory - deprecate, delegate to AHC object
  • play.api.libs.ws.ning.NingWSClientConfigParser - remove
  • play.api.libs.ws.ning.NingAsyncHttpClientConfigBuilder - deprecate, extends AHC class

NingWS.scala

  • NingWSClient - deprecate, extend AHC class
  • NingWSRequest - remove
  • NingWSModule - remove
  • WSClientProvider - remove
  • NingWSAPI - remove
  • NingWSCookie - remove
  • NingWSResponse - remove
  • NingWSComponents - deprecate, extend AHC class

My next question is: will there be a Play 2.6 w/ AHC? At some point, I expect AHC to be replaced by Akka HTTP.

I think most likely we will continue to use AHC in Play 2.6. We will probably move to Akka HTTP for both client/server in the future but it will take some work to get there and Akka HTTP might need some performance tuning or bug fixing first. I can imagine that will be 6+ months away, i.e. in Play 2.7 or later.

@slandelle
Copy link
Contributor Author

@richdougherty Thanks for the instructions. Will do in the next days.

@richdougherty
Copy link
Member

@slandelle excellent thanks. :)

@slandelle
Copy link
Contributor Author

@richdougherty I updated the PR. Could you please take a look?

@richdougherty
Copy link
Member

👍!

richdougherty added a commit that referenced this pull request Nov 30, 2015
@richdougherty richdougherty merged commit 0f8be93 into playframework:master Nov 30, 2015
@slandelle
Copy link
Contributor Author

Great, thanks!

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.

None yet

4 participants