Introduce and extend Config pattern (II)

[sazzad16]

Resolves #1780
Resolves #1824
Resolves #2234

Closes #1787
Closes #2237

Closes #2342
Closes #2349
Closes #2353

[mina-asham]

This looks really good I quite like the concept, I just have a few general comments/questions:

• Sorry if I spammed about repeated comments about javadoc, I don't know the convention for repeated comments in PR, let me know if there is a preferred way!
• I found the CR really really big, I think smaller more controller CRs can help a lot getting things moving a bit faster, this is just for the benefit of reviewers tbh, a few examples:
  • Moving to Java 8, this is not really essential to this PR, I only noticed one lambda usage which is not essential, could be better in a different PR
  • Deprecating constructors/setters/etc... could have been done in a separate PR too
  • Renaming/refactoring some tests that are not essential to this change

[sazzad16]

@mina-asham Thank you very much for your review. Please re-review as well :)

Moving to Java 8, this is not really essential to this PR, I only noticed one lambda usage which is not essential, could be better in a different PR

I need it to use default. I can do it separately, but in that case a review+merge would be required very fast.

Deprecating constructors/setters/etc... could have been done in a separate PR too

Most of the deprecations are directly effected by this PR.

[mina-asham]

@mina-asham Thank you very much for your review. Please re-review as well :)

Moving to Java 8, this is not really essential to this PR, I only noticed one lambda usage which is not essential, could be better in a different PR

I need it to use default. I can do it separately, but in that case a review+merge would be required very fast.

Deprecating constructors/setters/etc... could have been done in a separate PR too

Most of the deprecations are directly effected by this PR.

@sazzad16 that's fair enough, it's just a really big PR 😄

[mina-asham]

Thanks for updating, feel free to resolve all deprecation comments to clear out the clutter, I think the main bits here that I think we should change are (summarizing here because I feel like the deprecation comments made too much clutter):

• Drop builder getters, this is not really a common use case or a common pattern
• Clear up the circleci configuration from jdk7 references (just to avoid any confusion)
• DefaultJedisSocketConfig.withSsl I think the name is confusing see the suggestion there
• Some comments in tests
• Some formatting comments in some classes

Feel free to address any of these with just comments if you disagree about the suggestions :)

[sazzad16]

@mina-asham Please re-review.
I think I'm done except DefaultJedisSocketConfig.withSsl. Check my comment there. Waiting for you response.

[sazzad16]

@mina-asham Let's continue at #2368

[mina-asham]

@mina-asham Please re-review.
I think I'm done except DefaultJedisSocketConfig.withSsl. Check my comment there. Waiting for you response.

Thanks, commented on that, agree with your approach!

@mina-asham Let's continue at #2368

@sazzad16 Will do, just to make sure I am not misunderstanding anything though, we are abandoning this PR? (if so can you close it?)
Also, sorry for the delays between reviews, time is tight :)

[sazzad16]

@mina-asham

sorry for the delays between reviews, time is tight :)

Don't worry. I understand.

we are abandoning this PR?

Only if we agree on the new one. If we disregard that, we may have to come back here.