First of all, you edited way too many files (185!!!):

• You automatically renamed AsyncHttpClient into AsyncHttpClientImpl everywhere (IDE refactoring I guess) and that's useless in most places that should not be aware of the implementation (tests, etc)
• You don't use the proper formatting (we use spaces, you use tabs).

Then, what's the context of this? I discussed turning AsyncHttpClient into an interface with some people from eBay, are you related?
Lets discuss what you propose once there's less modified code so we can get a better picture of what you suggest.

Cheers,

Stéphane

Hello Stephane,
I am from the same team at eBay. AsyncHttpClient has been converted to an interface and the default implementation class is AsyncHttpClientImpl. I will modify the tests to use the interface once our proposal is accepted. I have formatted the code in order to use spaces and not tabs.

Here are the main implementation classes that I have added:
AsyncHttpClientFactory: The factory class that returns back an instance of AsyncHttpClient.
AsyncHttpClientRegistry: The interface for the registry class that caches an instance of AsyncHttpClient.
AsyncHttpClientRegistryImpl: The default implementation of Registry and it's getInstance() method returns back a pluggable implementation of the Registry.
AsyncImplHelper: A utilty class that helps in finding the implementation classes.

Here are the unit test classes:
AbstractAsyncHttpClientFactoryTest
GrizzlyAsyncHttpClientFactoryTest
NettyAsyncHttpClientFactoryTest
AsyncHttpClientRegistryTest
BadAsyncHttpClient
BadAsyncHttpClientRegistry
PA(Classes that help with unit tests. I can import the jar instead of adding individual classes if needed)
TestAsyncHttpClient
TestAsyncHttpClientRegistry
PrivilegedAccessor

The rest of the files were automatically refactored due to the change in the class name

I must insist that you first leave unchanged the files that shouldn't be impacted with the change you propose. There's currently 196 modified files, most of them shouldn't, and that's way too much for review.

Stephane, this is Mihai Poplacenel from eBay, and Salil is indeed my colleague, the one who's actually done the work :).

We'll try to reduce the number of files changed. However, in order to switch from class+constructor to interface+factory, the places where the constructor gets instantiated will have to be replaced, and those may be a non-trivial number. I was therefore wondering, are you also concerned about that number?

My current problem is that there's way too many modifications that are not related to the actual refactoring, but mostly due to the way the refactoring was handled: automatic renaming in the IDE (tests using the Impl instead of the interface), changes in formatting (dandling blanks, additional eol), etc:

196 changed files with 3,554 additions and 1,318 deletions: https://github.com/AsyncHttpClient/async-http-client/pull/487/files

All this noise makes it impossible to review as is, analyze what is exactly been done, and detect mistakes.

I understand, we'll reduce the noise and submit a new PR.

Thanks!
-- Mihai

You don't have to submit a new one. For now, just amend with new commits, and I'll ask you to rebase once it's ready to be merged.

Stephane, do you guys have a checkstyle settings (or for any other tool) that we can run and align to your formatting? It seems like we'd have to redo our work for us to revert the formatting changes...

No, we don't. The thing is Jean-François, Ryan and I have different coding styles (JFA uses vim, so he likes 80 chars lines, which I hate).
@jfarcand @rlubke Suggestion?

@callatis Use IDE checkstyle...I use IDEA as well.

I try, sometimes unsuccessfully, to follow the style of the source file I'm
editing (if not mine) - which means no global reformat and import
optimization. That said, we should probably look into check style for the
build.

I turned AsyncHttpClient into an interface, see #497.
Up to you to propose an optional Factory and Registry on top of this.

Hello Stephane,
I am working on a pull request with AsyncHttpClient changed to an interface with a default implementation.

That's exactly what I did, so you can focus on the other changes you propose.

I already have all the changes ready including changing the class to an
interface in my github repo and was about to make a pull request
https://github.com/salilsurendran/async-http-client/ . Would it be possible
to merge this pull request now or would I have to do a rebase?

On Wed, Mar 5, 2014 at 4:01 PM, Stephane Landelle
notifications@github.comwrote:

That's exactly what I did, so you can focus on the other changes you
propose.

Reply to this email directly or view it on GitHubhttps://github.com//pull/487#issuecomment-36810306
.

Thanks,
Salil
"The surest sign that intelligent life exists elsewhere in the universe is
that none of it has tried to contact us."

I have been able to merge in all your changes into my repo. I will make a
pull request soon.

On Wed, Mar 5, 2014 at 4:17 PM, Salil Surendran salilsurendran@gmail.comwrote:

I already have all the changes ready including changing the class to an
interface in my github repo and was about to make a pull request
https://github.com/salilsurendran/async-http-client/ . Would it be
possible to merge this pull request now or would I have to do a rebase?

On Wed, Mar 5, 2014 at 4:01 PM, Stephane Landelle <
notifications@github.com> wrote:

That's exactly what I did, so you can focus on the other changes you
propose.

Reply to this email directly or view it on GitHubhttps://github.com//pull/487#issuecomment-36810306
.

Thanks,
Salil
"The surest sign that intelligent life exists elsewhere in the universe is
that none of it has tried to contact us."

Thanks,
Salil
"The surest sign that intelligent life exists elsewhere in the universe is
that none of it has tried to contact us."

The pull request adding the Factory and Registry has been made.

On Thu, Mar 6, 2014 at 12:41 AM, Salil Surendran
salilsurendran@gmail.comwrote:

I have been able to merge in all your changes into my repo. I will make a
pull request soon.

On Wed, Mar 5, 2014 at 4:17 PM, Salil Surendran salilsurendran@gmail.comwrote:

I already have all the changes ready including changing the class to an
interface in my github repo and was about to make a pull request
https://github.com/salilsurendran/async-http-client/ . Would it be
possible to merge this pull request now or would I have to do a rebase?

On Wed, Mar 5, 2014 at 4:01 PM, Stephane Landelle <
notifications@github.com> wrote:

That's exactly what I did, so you can focus on the other changes you
propose.

Reply to this email directly or view it on GitHubhttps://github.com//pull/487#issuecomment-36810306
.

Thanks,
Salil
"The surest sign that intelligent life exists elsewhere in the universe
is that none of it has tried to contact us."

Thanks,
Salil
"The surest sign that intelligent life exists elsewhere in the universe is
that none of it has tried to contact us."

Thanks,
Salil
"The surest sign that intelligent life exists elsewhere in the universe is
that none of it has tried to contact us."