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

Some code cleanups #1888

Closed
wants to merge 7 commits into from
Closed

Some code cleanups #1888

wants to merge 7 commits into from

Conversation

gkorland
Copy link
Contributor

@gkorland gkorland commented Nov 5, 2018

  1. diamonds operator
  2. missing final
  3. index of with '' when possible

1. diamonds operator
2. missing final
3. index of with '' when possible
Copy link
Collaborator

@sazzad16 sazzad16 left a comment

Choose a reason for hiding this comment

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

Personally, I prefer nested enums to be declared static for the sake of readability. Let's see what @marcosnils says.

@gkorland
Copy link
Contributor Author

gkorland commented Nov 8, 2018

@sazzad16 but there is no need every nested enum is static

@gkorland gkorland added this to the 3.1.0 milestone Dec 6, 2018
@sazzad16 sazzad16 modified the milestones: 3.1.0, 3.2.0 Jul 17, 2019
@gkorland
Copy link
Contributor Author

@sazzad16 any more comments but the static enum?

@@ -20,7 +20,7 @@
public Pool() {
}

public Pool(final GenericObjectPoolConfig poolConfig, PooledObjectFactory<T> factory) {
public Pool(final GenericObjectPoolConfig<T> poolConfig, PooledObjectFactory<T> factory) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Won't this create backward compatibility issue?

@@ -33,7 +33,7 @@ public boolean isClosed() {
return this.internalPool.isClosed();
}

public void initPool(final GenericObjectPoolConfig poolConfig, PooledObjectFactory<T> factory) {
public void initPool(final GenericObjectPoolConfig<T> poolConfig, PooledObjectFactory<T> factory) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

and this?

@sazzad16
Copy link
Collaborator

@gkorland Except static enum? No, there aren't anything that I object.
Although I'm a bit concerned about GenericObjectPoolConfig -> GenericObjectPoolConfig<T> change. We may have to separate those changes for 4.0+.

@gkorland
Copy link
Contributor Author

I guess we can close this PR since we just merged #2100

@gkorland gkorland closed this Nov 25, 2019
@sazzad16
Copy link
Collaborator

@gkorland We could go further with one. #2100 has 64 changes. But this PR had many more.

@gkorland
Copy link
Contributor Author

I'll submit a new PR

@sazzad16
Copy link
Collaborator

okay

@sazzad16 sazzad16 removed this from the 3.2.0 milestone Nov 26, 2019
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

2 participants