Five refactorings #45
Conversation
… fields should not be static)
…s case suppress the warning since logging "to stdout" is the express purpose of the class SystemPrintLogger.
…s case suppress the warning since logging "to stdout" is the express purpose of the class SystemPrintLogger.
Unused constructors should be removed
|
The next set of refactorings is ready. |
| @@ -68,10 +68,10 @@ public DockerDNSRRDiscoveryStrategy( | |||
| return discoveryNodes; | |||
| } | |||
|
|
|||
| Set<InetAddress> serviceNameResolutions = | |||
| new HashSet<>(); | |||
| // TODO robin - tighten scope of these variables | |||
There was a problem hiding this comment.
Already done in a pending PR which tightens variable scopes.
| @@ -8,10 +8,9 @@ | |||
| import java.util.logging.Level; | |||
| import java.util.logging.LogRecord; | |||
|
|
|||
| @SuppressWarnings("squid:S106") | |||
There was a problem hiding this comment.
@SuppressWarnings is already used in this project.
There was a problem hiding this comment.
yes, but not referencing third party tooling like this which is not used in the project
There was a problem hiding this comment.
Understood. will remove as part of the next PR if that's ok.
|
Please just proceed with adding whatever other commits you have or are proposing to this PR if possible no need for a a bunch of different PRs if they are all just little things like this that are being dumped out of some automated code analyzer tool. thx |
| * @param logger | ||
| * @param properties | ||
| */ | ||
| public DockerSwarmDiscoveryStrategy(DiscoveryNode localDiscoveryNode, ILogger logger, Map<String, Comparable> properties) { |
There was a problem hiding this comment.
Do not alter/remove any public signatures/constructors. This could break others using this library in use-cases specific to them such as programmatically configuring the stack.
There was a problem hiding this comment.
The passed DiscoveryNode is never used. Surely best to clean that up with an appropriate release note? Happy to revert if you wish....
| @@ -36,7 +36,7 @@ public DiscoveryStrategy newDiscoveryStrategy(DiscoveryNode discoveryNode, | |||
| ILogger logger, | |||
| Map<String, Comparable> properties) { | |||
|
|
|||
| return new DockerSwarmDiscoveryStrategy(discoveryNode, logger, properties); | |||
There was a problem hiding this comment.
Do not alter/remove any public signatures/constructors. This could break others using this library in use-cases specific to them such as programmatically configuring the stack.
There was a problem hiding this comment.
The passed ILogger is never used. Surely best to clean that up with an appropriate release note? Happy to revert if you wish....
| @@ -46,9 +45,7 @@ | |||
| /** | |||
| * Constructor | |||
| */ | |||
|
|
|||
|
|
|||
| public SwarmAddressPicker(final ILogger iLogger) { | |||
There was a problem hiding this comment.
Do not alter/remove any public signatures/constructors. This could break others using this library in use-cases specific to them such as programmatically configuring the stack.
There was a problem hiding this comment.
The passed DiscoveryNode is never used. Surely best to clean that up with an appropriate release note? Happy to revert if you wish....
|
|
||
| // Since SwarmDiscoveryUtil is used by several components | ||
| // the context lets us distinguish instances in logs | ||
| private String context = null; | ||
|
|
||
| private ILogger logger = Logger.getLogger(SwarmDiscoveryUtil.class); | ||
|
|
||
| public SwarmDiscoveryUtil(String context, |
There was a problem hiding this comment.
Do not alter/remove any public signatures/constructors. This could break others using this library in use-cases specific to them such as programmatically configuring the stack.
There was a problem hiding this comment.
I don't believe that constructor has actually changed recently. I would, however, love to get rid of "context" as it makes log messages longer than would otherwise be the case. Of course it might be that from a support perspective having SwarmDiscoveryUtil[CallingClass] in the log message has proven helpful.
There was a problem hiding this comment.
Leave the context.
Don't have time at the moment to go through each these diffs in detail but perhaps they are presented in confusing manner in this UI. I just see red removal here of an entire public constructor.
In any case, I don't mind formatting changes but do not alter or remove any pre-existing public method signatures or public constructors, regardless of if this library is internally calling it or not. Other users of this library could be calling these methods/constructors for other uses and this will maintain backwards compatibility.
Factor out processing of comma-delimited strings of tokens and properties
Five refactorings: