Added commands for adding acceptors and connectors in messaging subsystem #53
Conversation
|
The Also, how often does one need to use these commands? They seem quite specific to me. |
|
What about |
|
+1 to those names. They're unique and, at the same time, not that longer. |
| import java.util.Map; | ||
|
|
||
| /** | ||
| * An abstract class which implements common properties of connector and acceptor in messaging connections. |
There was a problem hiding this comment.
You don't have to say that it's an abstract class, everyone can see that from abstract class ... below.
|
Good work on test coverage, by the way! |
|
Thank you! I added suggested improvements and fixed it. |
honza-kasik
force-pushed
the
messaging-connections-commands
branch
3 times, most recently
from
c0e741b to
c05e160
Compare
| protected final String factoryClass; | ||
|
|
||
|
|
||
| public AbstractTransportConfigAddCommand(TransportConfigType configType, TransportConfigItem configItem, |
|
I think this looks good now, with that single comment about privacy of a constructor. |
|
Oh, and one more thing: the |
|
OK, please squash and I'll merge. |
honza-kasik
force-pushed
the
messaging-connections-commands
branch
from
87a0c1f to
ef1cfd7
Compare
|
Commits were squashed |
|
Why two commits? Are the tests in the 2nd commit somehow different from the tests in the 1st commit? Also, why the note "Fixed bugs and added suggested improvements" in the first commit? |
honza-kasik
force-pushed
the
messaging-connections-commands
branch
2 times, most recently
from
e55cb84 to
84240c4
Compare
|
Sorry, fixed |
| this.factoryClass = factoryClass; | ||
| } | ||
|
|
||
| protected AbstractTransportConfigAddCommand(InVmBuilder builder, TransportConfigItem configItem) { |
There was a problem hiding this comment.
Can you parametrize reference to generic type with wildcard (i.e. InVmBuilder<?> builder) to prevent in this case IMO uneccessary compiler complaints?
There was a problem hiding this comment.
Sure I can, I just did not understand why to generify this before. 👍
There was a problem hiding this comment.
We already have quite a bunch of unchecked conversions in commands. They are practically unavoidable when using generics to simulate self types. I don't mind much, really. @honza-kasik it's your choice; in fact, I was just getting to merge this when @zebrik added these comments :-)
There was a problem hiding this comment.
Hah, took me too long to comment :-) Nevermind.
honza-kasik
force-pushed
the
messaging-connections-commands
branch
from
84240c4 to
618faa8
Compare
|
Parametrized contructor arguments with wildcard. |
|
I still don't understand why we have |
honza-kasik
force-pushed
the
messaging-connections-commands
branch
from
618faa8 to
f78a17a
Compare
|
I am really sorry, as I was squashing commits I totally overlooked this... |
Add offline tests for custom-*
No description provided.