Add ability to set attributes on a SslContext#9654
Conversation
Motivation: Sometimes it is useful to be able to set attributes on a SslContext. Modifications: Add new method that will return a AttributeMap that is tied to a SslContext instance Result: Fixes #6542.
|
Do you have an example of a use case? |
|
@slandelle #6542 would be an example.... that said it is basically the same as |
njhill
left a comment
There was a problem hiding this comment.
Did you consider having SslContext extend DefaultAttributeMap a la AbstractChannel? (I know there are downsides of doing that too)
| /** | ||
| * Returns the {@link AttributeMap} that is tied to this {@link SslContext} instance. | ||
| */ | ||
| public final AttributeMap attributeMap() { |
There was a problem hiding this comment.
WDYT about naming this attributes() instead of attributeMap()?
|
@njhill yeah I did consider but I thought how it is done here may be better.. WDYT ? |
|
@normanmaurer I don't have a strong opinion but would probably lean towards subclassing, other things being equal. In future if we wanted to extend a different class then DefaultAttributeMap could be moved to a field and just delegate the |
|
I'm a sucker for composition. The change is OK but my eye twitches about why |
|
@rkapsi fair enough... use |
|
@netty-bot test this please |
rkapsi
left a comment
There was a problem hiding this comment.
LGTM and thank you for considering composition over inheritance :)
Motivation: Sometimes it is useful to be able to set attributes on a SslContext. Modifications: Add new method that will return a AttributeMap that is tied to a SslContext instance Result: Fixes #6542.
Motivation:
Sometimes it is useful to be able to set attributes on a SslContext.
Modifications:
Add new method that will return a AttributeMap that is tied to a SslContext instance
Result:
Fixes #6542.