[bisq-network/bisq] Refactor option handling (#3889)

[sqrrm]

sqrrm requested changes on this pull request.

NACK

See comments on exception usage.

Comments on braces are not blocking but it would be good to address the multiline blocks.

Apart from those comments it looks ready to merge.

> +                        .withRequiredArg()
+                        .ofType(String.class)
+                        .describedAs("OFF|ALL|ERROR|WARN|INFO|DEBUG|TRACE")
+                        .defaultsTo(Level.INFO.levelStr);
+
+        ArgumentAcceptingOptionSpec<Path> torrcFileOpt =
+                parser.accepts("torrcFile", "An existing torrc-file to be sourced for Tor. Note that torrc-entries, " +
+                        "which are critical to Bisq's correct operation, cannot be overwritten.")
+                        .withRequiredArg()
+                        .withValuesConvertedBy(new PathConverter(PathProperties.FILE_EXISTING, PathProperties.READABLE));
+
+        try {
+            OptionSet cliOpts = parser.parse(args);
+
+            if (cliOpts.has(helpOpt))
+                throw new HelpRequested(parser);

This goes against our style of not using exceptions as a part of the expected execution flow.

> +                            ex.getMessage()));
+        }
+    }
+
+    private Optional<OptionSet> parseOptionsFrom(File file, OptionParser parser, OptionSpec<?>... disallowedOpts) {
+        if (!file.isAbsolute() || !file.exists())
+            return Optional.empty();
+
+        ConfigFileReader configFileReader = new ConfigFileReader(file);
+        String[] optionLines = configFileReader.getOptionLines().stream()
+                .map(o -> "--" + o) // prepend dashes expected by jopt parser below
+                .collect(toList())
+                .toArray(new String[]{});
+
+        OptionSet configFileOpts = parser.parse(optionLines);
+        for (OptionSpec<?> disallowedOpt : disallowedOpts)

Prefer braces...

> +        }
+    }
+
+    private Optional<OptionSet> parseOptionsFrom(File file, OptionParser parser, OptionSpec<?>... disallowedOpts) {
+        if (!file.isAbsolute() || !file.exists())
+            return Optional.empty();
+
+        ConfigFileReader configFileReader = new ConfigFileReader(file);
+        String[] optionLines = configFileReader.getOptionLines().stream()
+                .map(o -> "--" + o) // prepend dashes expected by jopt parser below
+                .collect(toList())
+                .toArray(new String[]{});
+
+        OptionSet configFileOpts = parser.parse(optionLines);
+        for (OptionSpec<?> disallowedOpt : disallowedOpts)
+            if (configFileOpts.has(disallowedOpt))

...for all blocks.

> +
+        OptionSet configFileOpts = parser.parse(optionLines);
+        for (OptionSpec<?> disallowedOpt : disallowedOpts)
+            if (configFileOpts.has(disallowedOpt))
+                throw new IllegalArgumentException(
+                        format("The '%s' option is disallowed in config files", disallowedOpt.options().get(0)));
+
+        return Optional.of(configFileOpts);
+    }
+
+    public String getDefaultAppName() {
+        return defaultAppName;
+    }
+
+    public File getDefaultUserDataDir() {
+        if (Utilities.isWindows())

Prefer braces even for single, unbroken line blocks, but I don't think we have completely agreed on that one.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/bisq-network/bisq/pull/3889#pullrequestreview-345477776
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20200121/d72384f6/attachment.html>