You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by GitBox <gi...@apache.org> on 2021/01/20 15:03:28 UTC
[GitHub] [ignite-3] kgusakov opened a new pull request #31: IGNITE-14000 Fix codestyle in cli and cli-common modules
kgusakov opened a new pull request #31:
URL: https://github.com/apache/ignite-3/pull/31
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] vkulichenko commented on a change in pull request #31: IGNITE-14000 Fix codestyle in cli and cli-common modules
Posted by GitBox <gi...@apache.org>.
vkulichenko commented on a change in pull request #31:
URL: https://github.com/apache/ignite-3/pull/31#discussion_r562976998
##########
File path: modules/cli/src/main/java/org/apache/ignite/cli/InteractiveWrapper.java
##########
@@ -38,8 +38,16 @@
import picocli.CommandLine;
import picocli.shell.jline3.PicocliCommands;
+/**
+ * Interactive shell mode for Ignite CLI.
+ */
public class InteractiveWrapper {
Review comment:
Remove the empty line.
##########
File path: modules/cli/src/main/java/org/apache/ignite/cli/spec/ConfigCommandSpec.java
##########
@@ -31,53 +34,75 @@
}
)
public class ConfigCommandSpec extends CategorySpec {
+ /**
+ * Command for get Ignite node configurations.
+ */
@CommandLine.Command(name = "get", description = "Gets current Ignite cluster configuration values.")
public static class GetConfigCommandSpec extends CommandSpec {
+ /** Configuration client for REST node API. */
+ @Inject
+ private ConfigurationClient configurationClient;
- @Inject private ConfigurationClient configurationClient;
-
- @CommandLine.Mixin CfgHostnameOptions cfgHostnameOptions;
+ /** Command option for setting custom node host. */
+ @CommandLine.Mixin
+ CfgHostnameOptions cfgHostnameOptions;
Review comment:
Are these fields supposed to be package-private? Unless this is a Picocli requirement, they should be private (please fix across all commands if needed).
##########
File path: modules/cli/src/main/java/org/apache/ignite/cli/builtins/config/HttpClientFactory.java
##########
@@ -21,9 +21,17 @@
import javax.inject.Singleton;
import io.micronaut.context.annotation.Factory;
+/**
+ * Factory for producing simple HTTP clients.
+ */
@Factory
public class HttpClientFactory {
Review comment:
Remove empty line.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] sk0x50 commented on a change in pull request #31: IGNITE-14000 Fix codestyle in cli and cli-common modules
Posted by GitBox <gi...@apache.org>.
sk0x50 commented on a change in pull request #31:
URL: https://github.com/apache/ignite-3/pull/31#discussion_r561657765
##########
File path: modules/cli/src/main/java/org/apache/ignite/cli/CliPathsConfigLoader.java
##########
@@ -27,51 +27,96 @@
import javax.inject.Singleton;
import org.apache.ignite.cli.builtins.SystemPathResolver;
+/**
+ * Due to the nature of Ignite CLI tool it can be runned in the environment without ignitecfg file at all.
+ * This class created to simplify the managing of the different cases:
+ * <ul>
+ * <li>When user download binary and run it to manage any existence remote cluster</li>
+ * <li>When user download binary and run 'init' to deploy Ignite distribution on the current machine</li>
+ * <li>When user install it by system package</li>
+ * </ul>
+ */
@Singleton
public class CliPathsConfigLoader {
+ /** System paths' resolver. */
+ private final SystemPathResolver pathRslvr;
- private final SystemPathResolver pathResolver;
- private final String version;
+ /** Ignite CLI tool version. */
+ private final String ver;
+ /**
+ * Creates new loader.
+ *
+ * @param pathRslvr System paths' resolver.
+ * @param cliVerInfo CLI version info provider.
+ */
@Inject
- public CliPathsConfigLoader(SystemPathResolver pathResolver,
- CliVersionInfo cliVersionInfo) {
- this.pathResolver = pathResolver;
- this.version = cliVersionInfo.version;
+ public CliPathsConfigLoader(
+ SystemPathResolver pathRslvr,
+ CliVersionInfo cliVerInfo
+ ) {
+ this.pathRslvr = pathRslvr;
+ this.ver = cliVerInfo.ver;
}
+ /**
+ * Loads Ignite paths config from file if exists.
+ *
+ * @return IgnitePaths if config file exists, empty otherwise.
+ */
public Optional<IgnitePaths> loadIgnitePathsConfig() {
if (configFilePath().toFile().exists())
- return Optional.of(CliPathsConfigLoader.readConfigFile(configFilePath(), version));
+ return Optional.of(readConfigFile(configFilePath(), ver));
return Optional.empty();
}
+ /**
+ * Loads Ignite paths configuration if config file exists or failed otherwise.
+ *
+ * @return IgnitePaths or throw exception, if no config file exists.
+ */
Review comment:
Perhaps, it would be good to explicitly declare the type of Exception: `@throws IgniteCLIException If the configuration file does not exist.`
##########
File path: modules/cli/src/main/java/org/apache/ignite/cli/IgnitePaths.java
##########
@@ -20,43 +20,96 @@
import java.io.File;
import java.nio.file.Path;
+/**
+ * The main resolver of Ignite paths for the current installation (like bin, work and etc. dirs).
+ * Current Ignite distributive has the following dirs structure:
+ * <ul>
+ * <li>bin</li>
+ * <ul>
+ * <li>${version}</li>
+ * <ul>
+ * <li>cli</li>
+ * <li>libs</li>
+ * </ul>
+ * </ul>
+ * <li>work</li>
+ * <ul>
+ * <li>config</li>
+ * <ul>
+ * <li>default-config.xml</li>
+ * </ul>
+ * <li>cli</li>
+ * <ul>
+ * <li>pids</li>
+ * </ul>
+ * <li>modules.json</li>
+ * </ul>
+ * </ul>
+ */
public class IgnitePaths {
-
+ /**
+ * Path to Ignite bin directory.
+ * Bin directory contains jar artifacts for Ignite server and CLI modules.
+ */
public final Path binDir;
+
+ /**
+ * Work directory for Ignite server and CLI operation.
+ */
public final Path workDir;
- private final String version;
- public IgnitePaths(Path binDir, Path workDir, String version) {
+ /**
+ * Ignite CLI version.
+ * Also, the same version will be used for addressing any binaries inside bin dir
+ */
+ private final String ver;
+
+ /**
+ * Creates resolved ignite paths instance from Ignite CLI version and base dirs paths.
+ *
+ * @param binDir Bin directory.
+ * @param workDir Work directory.
+ * @param ver Ignite CLI version.
+ */
+ public IgnitePaths(Path binDir, Path workDir, String ver) {
this.binDir = binDir;
this.workDir = workDir;
- this.version = version;
+ this.ver = ver;
}
-
+ /** Path where CLI module artifacts will be placed. */
Review comment:
One-line Javadoc comments allowed and preferred for fields. I'm not sure that this can be applied to methods.
##########
File path: modules/cli/src/main/java/org/apache/ignite/cli/IgnitePaths.java
##########
@@ -20,43 +20,96 @@
import java.io.File;
import java.nio.file.Path;
+/**
+ * The main resolver of Ignite paths for the current installation (like bin, work and etc. dirs).
+ * Current Ignite distributive has the following dirs structure:
+ * <ul>
+ * <li>bin</li>
+ * <ul>
+ * <li>${version}</li>
+ * <ul>
+ * <li>cli</li>
+ * <li>libs</li>
+ * </ul>
+ * </ul>
+ * <li>work</li>
+ * <ul>
+ * <li>config</li>
+ * <ul>
+ * <li>default-config.xml</li>
+ * </ul>
+ * <li>cli</li>
+ * <ul>
+ * <li>pids</li>
+ * </ul>
+ * <li>modules.json</li>
+ * </ul>
+ * </ul>
+ */
public class IgnitePaths {
-
+ /**
+ * Path to Ignite bin directory.
+ * Bin directory contains jar artifacts for Ignite server and CLI modules.
+ */
public final Path binDir;
+
+ /**
+ * Work directory for Ignite server and CLI operation.
Review comment:
This javadoc can be a one-liner.
##########
File path: modules/cli/src/main/java/org/apache/ignite/cli/CliPathsConfigLoader.java
##########
@@ -27,51 +27,96 @@
import javax.inject.Singleton;
import org.apache.ignite.cli.builtins.SystemPathResolver;
+/**
+ * Due to the nature of Ignite CLI tool it can be runned in the environment without ignitecfg file at all.
+ * This class created to simplify the managing of the different cases:
+ * <ul>
+ * <li>When user download binary and run it to manage any existence remote cluster</li>
+ * <li>When user download binary and run 'init' to deploy Ignite distribution on the current machine</li>
+ * <li>When user install it by system package</li>
+ * </ul>
+ */
@Singleton
public class CliPathsConfigLoader {
+ /** System paths' resolver. */
+ private final SystemPathResolver pathRslvr;
- private final SystemPathResolver pathResolver;
- private final String version;
+ /** Ignite CLI tool version. */
+ private final String ver;
+ /**
+ * Creates new loader.
+ *
+ * @param pathRslvr System paths' resolver.
+ * @param cliVerInfo CLI version info provider.
+ */
@Inject
- public CliPathsConfigLoader(SystemPathResolver pathResolver,
- CliVersionInfo cliVersionInfo) {
- this.pathResolver = pathResolver;
- this.version = cliVersionInfo.version;
+ public CliPathsConfigLoader(
+ SystemPathResolver pathRslvr,
+ CliVersionInfo cliVerInfo
+ ) {
+ this.pathRslvr = pathRslvr;
+ this.ver = cliVerInfo.ver;
}
+ /**
+ * Loads Ignite paths config from file if exists.
+ *
+ * @return IgnitePaths if config file exists, empty otherwise.
+ */
public Optional<IgnitePaths> loadIgnitePathsConfig() {
if (configFilePath().toFile().exists())
- return Optional.of(CliPathsConfigLoader.readConfigFile(configFilePath(), version));
+ return Optional.of(readConfigFile(configFilePath(), ver));
return Optional.empty();
}
+ /**
+ * Loads Ignite paths configuration if config file exists or failed otherwise.
+ *
+ * @return IgnitePaths or throw exception, if no config file exists.
+ */
public IgnitePaths loadIgnitePathsOrThrowError() {
Optional<IgnitePaths> ignitePaths = loadIgnitePathsConfig();
+
if (ignitePaths.isPresent()) {
if (!ignitePaths.get().validateDirs())
Review comment:
Even though there is only one instruction under this condition, it requires brackets because it occupies two lines.
##########
File path: modules/cli/src/main/java/org/apache/ignite/cli/CommandFactory.java
##########
@@ -21,16 +21,25 @@
import io.micronaut.context.ApplicationContext;
import picocli.CommandLine;
+/**
+ * Picocli command factory for initialize commands and DI dependencies.
+ */
public class CommandFactory implements CommandLine.IFactory {
+ /** DI application context. */
+ private final ApplicationContext applicationCtx;
- private final ApplicationContext applicationContext;
-
- public CommandFactory(ApplicationContext applicationContext) {
- this.applicationContext = applicationContext;
+ /**
+ * Creates new command factory.
+ *
+ * @param applicationCtx DI application context.
+ */
+ public CommandFactory(ApplicationContext applicationCtx) {
+ this.applicationCtx = applicationCtx;
}
+ /** {inheritDoc} */
Review comment:
Please change it to `{@inheritDoc}` here and below.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] sk0x50 commented on a change in pull request #31: IGNITE-14000 Fix codestyle in cli and cli-common modules
Posted by GitBox <gi...@apache.org>.
sk0x50 commented on a change in pull request #31:
URL: https://github.com/apache/ignite-3/pull/31#discussion_r561657765
##########
File path: modules/cli/src/main/java/org/apache/ignite/cli/CliPathsConfigLoader.java
##########
@@ -27,51 +27,96 @@
import javax.inject.Singleton;
import org.apache.ignite.cli.builtins.SystemPathResolver;
+/**
+ * Due to the nature of Ignite CLI tool it can be runned in the environment without ignitecfg file at all.
+ * This class created to simplify the managing of the different cases:
+ * <ul>
+ * <li>When user download binary and run it to manage any existence remote cluster</li>
+ * <li>When user download binary and run 'init' to deploy Ignite distribution on the current machine</li>
+ * <li>When user install it by system package</li>
+ * </ul>
+ */
@Singleton
public class CliPathsConfigLoader {
+ /** System paths' resolver. */
+ private final SystemPathResolver pathRslvr;
- private final SystemPathResolver pathResolver;
- private final String version;
+ /** Ignite CLI tool version. */
+ private final String ver;
+ /**
+ * Creates new loader.
+ *
+ * @param pathRslvr System paths' resolver.
+ * @param cliVerInfo CLI version info provider.
+ */
@Inject
- public CliPathsConfigLoader(SystemPathResolver pathResolver,
- CliVersionInfo cliVersionInfo) {
- this.pathResolver = pathResolver;
- this.version = cliVersionInfo.version;
+ public CliPathsConfigLoader(
+ SystemPathResolver pathRslvr,
+ CliVersionInfo cliVerInfo
+ ) {
+ this.pathRslvr = pathRslvr;
+ this.ver = cliVerInfo.ver;
}
+ /**
+ * Loads Ignite paths config from file if exists.
+ *
+ * @return IgnitePaths if config file exists, empty otherwise.
+ */
public Optional<IgnitePaths> loadIgnitePathsConfig() {
if (configFilePath().toFile().exists())
- return Optional.of(CliPathsConfigLoader.readConfigFile(configFilePath(), version));
+ return Optional.of(readConfigFile(configFilePath(), ver));
return Optional.empty();
}
+ /**
+ * Loads Ignite paths configuration if config file exists or failed otherwise.
+ *
+ * @return IgnitePaths or throw exception, if no config file exists.
+ */
Review comment:
Perhaps, it would be good to explicitly declare the type of Exception: `@throws IgniteCLIException If the configuration file does not exist.`
##########
File path: modules/cli/src/main/java/org/apache/ignite/cli/IgnitePaths.java
##########
@@ -20,43 +20,96 @@
import java.io.File;
import java.nio.file.Path;
+/**
+ * The main resolver of Ignite paths for the current installation (like bin, work and etc. dirs).
+ * Current Ignite distributive has the following dirs structure:
+ * <ul>
+ * <li>bin</li>
+ * <ul>
+ * <li>${version}</li>
+ * <ul>
+ * <li>cli</li>
+ * <li>libs</li>
+ * </ul>
+ * </ul>
+ * <li>work</li>
+ * <ul>
+ * <li>config</li>
+ * <ul>
+ * <li>default-config.xml</li>
+ * </ul>
+ * <li>cli</li>
+ * <ul>
+ * <li>pids</li>
+ * </ul>
+ * <li>modules.json</li>
+ * </ul>
+ * </ul>
+ */
public class IgnitePaths {
-
+ /**
+ * Path to Ignite bin directory.
+ * Bin directory contains jar artifacts for Ignite server and CLI modules.
+ */
public final Path binDir;
+
+ /**
+ * Work directory for Ignite server and CLI operation.
+ */
public final Path workDir;
- private final String version;
- public IgnitePaths(Path binDir, Path workDir, String version) {
+ /**
+ * Ignite CLI version.
+ * Also, the same version will be used for addressing any binaries inside bin dir
+ */
+ private final String ver;
+
+ /**
+ * Creates resolved ignite paths instance from Ignite CLI version and base dirs paths.
+ *
+ * @param binDir Bin directory.
+ * @param workDir Work directory.
+ * @param ver Ignite CLI version.
+ */
+ public IgnitePaths(Path binDir, Path workDir, String ver) {
this.binDir = binDir;
this.workDir = workDir;
- this.version = version;
+ this.ver = ver;
}
-
+ /** Path where CLI module artifacts will be placed. */
Review comment:
One-line Javadoc comments allowed and preferred for fields. I'm not sure that this can be applied to methods.
##########
File path: modules/cli/src/main/java/org/apache/ignite/cli/IgnitePaths.java
##########
@@ -20,43 +20,96 @@
import java.io.File;
import java.nio.file.Path;
+/**
+ * The main resolver of Ignite paths for the current installation (like bin, work and etc. dirs).
+ * Current Ignite distributive has the following dirs structure:
+ * <ul>
+ * <li>bin</li>
+ * <ul>
+ * <li>${version}</li>
+ * <ul>
+ * <li>cli</li>
+ * <li>libs</li>
+ * </ul>
+ * </ul>
+ * <li>work</li>
+ * <ul>
+ * <li>config</li>
+ * <ul>
+ * <li>default-config.xml</li>
+ * </ul>
+ * <li>cli</li>
+ * <ul>
+ * <li>pids</li>
+ * </ul>
+ * <li>modules.json</li>
+ * </ul>
+ * </ul>
+ */
public class IgnitePaths {
-
+ /**
+ * Path to Ignite bin directory.
+ * Bin directory contains jar artifacts for Ignite server and CLI modules.
+ */
public final Path binDir;
+
+ /**
+ * Work directory for Ignite server and CLI operation.
Review comment:
This javadoc can be a one-liner.
##########
File path: modules/cli/src/main/java/org/apache/ignite/cli/CliPathsConfigLoader.java
##########
@@ -27,51 +27,96 @@
import javax.inject.Singleton;
import org.apache.ignite.cli.builtins.SystemPathResolver;
+/**
+ * Due to the nature of Ignite CLI tool it can be runned in the environment without ignitecfg file at all.
+ * This class created to simplify the managing of the different cases:
+ * <ul>
+ * <li>When user download binary and run it to manage any existence remote cluster</li>
+ * <li>When user download binary and run 'init' to deploy Ignite distribution on the current machine</li>
+ * <li>When user install it by system package</li>
+ * </ul>
+ */
@Singleton
public class CliPathsConfigLoader {
+ /** System paths' resolver. */
+ private final SystemPathResolver pathRslvr;
- private final SystemPathResolver pathResolver;
- private final String version;
+ /** Ignite CLI tool version. */
+ private final String ver;
+ /**
+ * Creates new loader.
+ *
+ * @param pathRslvr System paths' resolver.
+ * @param cliVerInfo CLI version info provider.
+ */
@Inject
- public CliPathsConfigLoader(SystemPathResolver pathResolver,
- CliVersionInfo cliVersionInfo) {
- this.pathResolver = pathResolver;
- this.version = cliVersionInfo.version;
+ public CliPathsConfigLoader(
+ SystemPathResolver pathRslvr,
+ CliVersionInfo cliVerInfo
+ ) {
+ this.pathRslvr = pathRslvr;
+ this.ver = cliVerInfo.ver;
}
+ /**
+ * Loads Ignite paths config from file if exists.
+ *
+ * @return IgnitePaths if config file exists, empty otherwise.
+ */
public Optional<IgnitePaths> loadIgnitePathsConfig() {
if (configFilePath().toFile().exists())
- return Optional.of(CliPathsConfigLoader.readConfigFile(configFilePath(), version));
+ return Optional.of(readConfigFile(configFilePath(), ver));
return Optional.empty();
}
+ /**
+ * Loads Ignite paths configuration if config file exists or failed otherwise.
+ *
+ * @return IgnitePaths or throw exception, if no config file exists.
+ */
public IgnitePaths loadIgnitePathsOrThrowError() {
Optional<IgnitePaths> ignitePaths = loadIgnitePathsConfig();
+
if (ignitePaths.isPresent()) {
if (!ignitePaths.get().validateDirs())
Review comment:
Even though there is only one instruction under this condition, it requires brackets because it occupies two lines.
##########
File path: modules/cli/src/main/java/org/apache/ignite/cli/CommandFactory.java
##########
@@ -21,16 +21,25 @@
import io.micronaut.context.ApplicationContext;
import picocli.CommandLine;
+/**
+ * Picocli command factory for initialize commands and DI dependencies.
+ */
public class CommandFactory implements CommandLine.IFactory {
+ /** DI application context. */
+ private final ApplicationContext applicationCtx;
- private final ApplicationContext applicationContext;
-
- public CommandFactory(ApplicationContext applicationContext) {
- this.applicationContext = applicationContext;
+ /**
+ * Creates new command factory.
+ *
+ * @param applicationCtx DI application context.
+ */
+ public CommandFactory(ApplicationContext applicationCtx) {
+ this.applicationCtx = applicationCtx;
}
+ /** {inheritDoc} */
Review comment:
Please change it to `{@inheritDoc}` here and below.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] vkulichenko merged pull request #31: IGNITE-14000 Fix codestyle in cli and cli-common modules
Posted by GitBox <gi...@apache.org>.
vkulichenko merged pull request #31:
URL: https://github.com/apache/ignite-3/pull/31
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] vkulichenko merged pull request #31: IGNITE-14000 Fix codestyle in cli and cli-common modules
Posted by GitBox <gi...@apache.org>.
vkulichenko merged pull request #31:
URL: https://github.com/apache/ignite-3/pull/31
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org