Rework cmdline #6
Conversation
| private static final Logger LOG = LoggerFactory.getLogger(EthFirewallConfig.class); | ||
|
|
||
| private final EthFirewallConfig config; | ||
| private RunnerBuilder runnerBuilder; |
There was a problem hiding this comment.
Couldn't the RunnerBuilder be final too?
|
|
||
| public RunnerBuilder() {} | ||
|
|
||
| public TransactionSigner getTransactionSigner() { |
There was a problem hiding this comment.
Why have getters in the RunnerBuilder? ...are they solely for testing?
There was a problem hiding this comment.
its a bit unwarranted ....
| return serverOptions; | ||
| } | ||
|
|
||
| public void setTransactionSigner(TransactionSigner transactionSigner) { |
There was a problem hiding this comment.
nit; method arguments can be final
|
|
||
| public final class EthFirewall { | ||
| private static final int ERROR_EXIT_CODE = 1; | ||
|
|
||
| private static final Logger LOG = LoggerFactory.getLogger(EthFirewallCommandLineConfig.class); |
There was a problem hiding this comment.
Log class is wrong, should be EthFirewall.class. Maybe we should have just used log4j directly instead of slf4j
There was a problem hiding this comment.
Done - happy to keep trying with slf4j
| byte[] fileContent = Files.readAllBytes(Paths.get(config.getPasswordFilePath())); | ||
| return Optional.of(new String(fileContent, Charsets.UTF_8)); | ||
| } catch (IOException ex) { | ||
| return Optional.empty(); |
There was a problem hiding this comment.
perhaps we should log this exception
There was a problem hiding this comment.
Done (but only at debug level).
|
|
||
| Level getLogLevel(); | ||
|
|
||
| String getPasswordFilePath(); |
There was a problem hiding this comment.
Perhaps use Path to for the password file path
| import io.vertx.core.http.HttpServerOptions; | ||
| import io.vertx.ext.web.client.WebClientOptions; | ||
| import org.apache.logging.log4j.core.config.Configurator; | ||
| import org.slf4j.Logger; |
There was a problem hiding this comment.
I recall something about Log4j2 vs SLF4j now ...as of mid 2018 there's no value brought by SLF4J
https://stackoverflow.com/questions/41498021/is-it-worth-to-use-slf4j-with-log4j2/41500347#41500347
https://stackoverflow.com/questions/41633278/can-we-use-all-features-of-log4j2-if-we-use-it-along-with-slf4j-api/41635246#41635246
SLF4J It's actually an interesting story, as it was created by a disgruntled Log4J contributor who wanted to move things in a new scary direction of abstraction (which after being proven right, Log4J adopted)
There was a problem hiding this comment.
Log4j2.x has adopted all features of SLF4J
https://en.wikipedia.org/wiki/SLF4J
...I mean Wikipedia wouldn't tell a lie 😉
There was a problem hiding this comment.
The single reason to use SLF4J over Log4J (or any other specific implementation) is that the backend is pluggable. For a standalone product that's pretty irrelevant but as soon as the code is embedded in another app a pluggable logging framework is pretty much essential.
There was a problem hiding this comment.
Oh and because SLF4J's API works properly if you do:
} catch (Exception e) {
LOG.error(e);
}
whereas Log4j will lose the stack trace.
There was a problem hiding this comment.
I see no reason not to continue using slf4j unless it causes problems. It also plays nicer with logging from dependent libraries as it has bridges for most logging frameworks and many open source frameworks use it anyway.
There was a problem hiding this comment.
@ajsutton @jframe since Log4j2 the backend is also pluggable, the default being Log4j but also supported is SLF4J, Apache Commons etc
https://logging.apache.org/log4j/2.x/index.html
Remove bintray publish CI step as not needed
Rework commandline to be a "config" struct, and move the actually application construction into the EthFirewall class.