Fix RunnerBuilder method names #1284
Conversation
| .discoveryHost(discoveryHost) | ||
| .discoveryPort(discoveryPort) | ||
| .advertisedHost(discoveryHost) | ||
| .listenPort(discoveryPort) |
There was a problem hiding this comment.
Is it weird to have the names mismatching here?
There was a problem hiding this comment.
Yep, thats weird :) . Updated the variable names. Also updated the description of the corresponding options.
| return this; | ||
| } | ||
|
|
||
| public RunnerBuilder discoveryPort(final int listenPort) { | ||
| public RunnerBuilder listenPort(final int listenPort) { |
There was a problem hiding this comment.
listenPort feels too generic here. I wonder if these should be p2pAdvertisedHost and p2pListenPort? A bigger refactoring would be to make an actual P2pConfiguration object to match JsonRpcConfiguration and friends but that's probably scope creep on what you were working on. :)
There was a problem hiding this comment.
Updated the names. We do have a NetworkingConfiguration object, although it's probably due for some cleanup.
| @@ -239,14 +239,14 @@ void setBootnodes(final List<String> values) { | |||
| @Option( | |||
| names = {"--p2p-host"}, | |||
| paramLabel = MANDATORY_HOST_FORMAT_HELP, | |||
| description = "Host for P2P peer discovery to listen on (default: ${DEFAULT-VALUE})", | |||
| description = "Ip address this node advertises to its peers (default: ${DEFAULT-VALUE})", | |||
There was a problem hiding this comment.
fyi @PegaSysEng/pliny - I'm updating the CLI descriptions here and below. Let me know if you see any issues.
PR description
I noticed the method name
RunnerBuilder.discoveryPortis inaccurate because it actually defines the listening port for both wire-level tcp connections and discovery-level udp messages. Updated this method name to be more generic. And also porteddiscoveryHostfor symmetry.