Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add the possibility to show the default values for parameters #135

1 change: 1 addition & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
- Add `.sys!perform()` and `.sys!slot()` to ForbiddenCallCheck.
- Do not check abstract method parameters in UnusedVariableCheck if `check-parameters`.
- Fix sslr-magik-toolkit's "Evaluate XPath"-button.
- Add `--with-default-values` option to `magik-lint`/`magik-typed-lint` to show the default values for parameters.
- Several fixes.

## Breaking changes (reiterated from above)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import java.util.Set;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import java.util.stream.Collectors;
import org.sonar.check.Rule;

/** MagicCheck holder/factory. */
Expand All @@ -21,31 +22,51 @@ public static class Parameter {
private final String name;
private final String description;
private final Object value;
private final Object defaultValue;

/**
* Constructor.
*
* @param name Name of parameter.
* @param description Description of parameter.
* @param value Value of parameter.
* @param defaultValue Default value of parameter.
*/
public Parameter(final String name, final String description, final @Nullable Object value) {
public Parameter(
final String name,
final String description,
final @Nullable Object value,
final @Nullable Object defaultValue) {
this.name = name;
this.description = description;
this.value = value;
this.defaultValue = defaultValue;
}

public String getName() {
return this.name;
}

public String getNameWithoutCheckName() {
return this.name.split("\\.")[1];
}

sebastiaanspeck marked this conversation as resolved.
Show resolved Hide resolved
public String getDescription() {
return this.description;
}

@CheckForNull
public Object getValue() {
return this.value;
return this.value != null ? this.value : this.defaultValue;
}

@CheckForNull
public Object getDefaultValue() {
return this.defaultValue;
}

public boolean isExplicitlySet() {
return this.value != null;
}
}

Expand All @@ -71,6 +92,10 @@ public MagikCheckHolder(
this.metadata = null;
}

private Set<Parameter> explicitlySetParameters() {
return this.parameters.stream().filter(Parameter::isExplicitlySet).collect(Collectors.toSet());
}

/**
* Get the wrapped check.
*
Expand All @@ -81,7 +106,8 @@ public MagikCheck createCheck() throws ReflectiveOperationException {
final MagikCheck check = this.checkClass.getDeclaredConstructor().newInstance();
check.setHolder(this);

for (final Parameter parameter : this.parameters) {
final Set<Parameter> explicitlySetParameters = this.explicitlySetParameters();
for (final Parameter parameter : explicitlySetParameters) {
final String name = parameter.getName();
final Object value = parameter.getValue();
check.setParameter(name, value);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,31 +62,28 @@ public List<MagikCheckHolder> getAllChecks() {
ruleProperty -> {
final String propertyKey = MagikChecksConfiguration.propertyKey(ruleProperty);
final String configKey = checkKey + "." + propertyKey;
if (!this.properties.hasProperty(configKey)) {
return null;
}

// Store parameter.
final String description = ruleProperty.description();
final MagikCheckHolder.Parameter parameter;
if (ruleProperty.type().equals("INTEGER")) {
final Integer configValue = this.properties.getPropertyInteger(configKey);
parameter =
new MagikCheckHolder.Parameter(configKey, description, configValue);
} else if (ruleProperty.type().equals("STRING")) {
final String configValue = this.properties.getPropertyString(configKey);
parameter =
new MagikCheckHolder.Parameter(configKey, description, configValue);
} else if (ruleProperty.type().equals("BOOLEAN")) {
final Boolean configValue = this.properties.getPropertyBoolean(configKey);
parameter =
new MagikCheckHolder.Parameter(configKey, description, configValue);
} else {
throw new IllegalStateException(
"Unknown type for property: " + ruleProperty.type());
Object configValue = null;
Object defaultValue = ruleProperty.defaultValue();

// Get configured value if it exists
if (this.properties.hasProperty(configKey)) {
if (ruleProperty.type().equals("INTEGER")) {
configValue = this.properties.getPropertyInteger(configKey);
defaultValue = Integer.parseInt(ruleProperty.defaultValue());
} else if (ruleProperty.type().equals("STRING")) {
configValue = this.properties.getPropertyString(configKey);
} else if (ruleProperty.type().equals("BOOLEAN")) {
configValue = this.properties.getPropertyBoolean(configKey);
defaultValue = Boolean.parseBoolean(ruleProperty.defaultValue());
} else {
throw new IllegalStateException(
"Unknown type for property: " + ruleProperty.type());
}
}

return parameter;
return new MagikCheckHolder.Parameter(
configKey, description, configValue, defaultValue);
})
.filter(Objects::nonNull)
.collect(Collectors.toSet());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,14 +86,15 @@ private List<MagikIssue> runCheckOnFile(final MagikFile magikFile, final MagikCh
}

/**
* Show checks active and inactive checks.
* Show enabled and disabled checks.
*
* @param writer Writer Write to write output to.
* @param showDisabled boolean Boolean to show disabled checks or not.
* @param showDefaultValues boolean Boolean to show default values for parameters or not.
* @throws ReflectiveOperationException -
* @throws IOException -
*/
void showChecks(final Writer writer, final boolean showDisabled)
void showChecks(final Writer writer, final boolean showDisabled, boolean showDefaultValues)
throws ReflectiveOperationException, IOException {
final MagikChecksConfiguration checksConfig =
new MagikChecksConfiguration(CheckList.getChecks(), this.properties);
Expand All @@ -106,14 +107,51 @@ void showChecks(final Writer writer, final boolean showDisabled)
continue;
}

for (final MagikCheckHolder.Parameter parameter : holder.getParameters()) {
showParameters(writer, holder, showDefaultValues);
sebastiaanspeck marked this conversation as resolved.
Show resolved Hide resolved
sebastiaanspeck marked this conversation as resolved.
Show resolved Hide resolved
}
}

/**
* Show parameters for a check.
*
* @param writer Writer Write to write output to.
* @param showDefaultValues boolean Boolean to show default values for parameters or not.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to state that it is a boolean. Java includes types for their parameters etc. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was copied from the method above (showChecks), but I probably introduced that as well.

* @throws ReflectiveOperationException -
* @throws IOException -
*/
private void showParameters(
final Writer writer, final MagikCheckHolder holder, final boolean showDefaultValues)
throws ReflectiveOperationException, IOException {
for (final MagikCheckHolder.Parameter parameter : holder.getParameters()) {
if (!parameter.isExplicitlySet() && !showDefaultValues) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if no value is set, and the --with-default-values is not given, the parameter will not be shown? That seems a bit confusing

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was the behaviour before this PR as well. Only user-changed parameters were shown. I kept that behaviour, but if a user uses --with-default-values, the default values will be shown as well.

continue;
}
final String defaultIndicator = !parameter.isExplicitlySet() ? " (default)" : "";
// Handle comma-separated list parameters differently
if (parameter.getValue() != null && parameter.getValue().toString().contains(",")) {
writer.write(
"\t"
+ parameter.getName()
+ ":\t"
+ parameter.getValue()
" ".repeat(2)
+ "*"
+ defaultIndicator
+ " "
+ parameter.getNameWithoutCheckName()
sebastiaanspeck marked this conversation as resolved.
Show resolved Hide resolved
+ " ("
+ parameter.getDescription()
+ "):\n");
String[] values = parameter.getValue().toString().split(",");
for (String value : values) {
writer.write(" ".repeat(4) + "- " + value.trim() + "\n");
}
} else {
writer.write(
" ".repeat(2)
+ "*"
+ defaultIndicator
+ " "
+ "("
+ parameter.getNameWithoutCheckName()
sebastiaanspeck marked this conversation as resolved.
Show resolved Hide resolved
+ ": "
+ parameter.getValue()
+ " ("
+ parameter.getDescription()
+ ")\n");
}
Expand All @@ -124,24 +162,28 @@ void showChecks(final Writer writer, final boolean showDisabled)
* Show enabled checks.
*
* @param writer Writer Write to write output to.
* @param showDefaultValues boolean Boolean to show default values for parameters or not.
* @throws ReflectiveOperationException -
* @throws IOException -
*/
void showEnabledChecks(final Writer writer) throws ReflectiveOperationException, IOException {
void showEnabledChecks(final Writer writer, final Boolean showDefaultValues)
throws ReflectiveOperationException, IOException {
writer.write("Enabled checks:\n");
this.showChecks(writer, false);
this.showChecks(writer, false, showDefaultValues);
}

/**
* Show disabled checks.
*
* @param writer Writer Write to write output to.
* @param showDefaultValues boolean Boolean to show default values for parameters or not.
* @throws ReflectiveOperationException -
* @throws IOException -
*/
void showDisabledChecks(final Writer writer) throws ReflectiveOperationException, IOException {
void showDisabledChecks(final Writer writer, final boolean showDefaultValues)
throws ReflectiveOperationException, IOException {
writer.write("Disabled checks:\n");
this.showChecks(writer, true);
this.showChecks(writer, true, showDefaultValues);
}

/**
Expand Down
65 changes: 42 additions & 23 deletions magik-lint/src/main/java/nl/ramsolutions/sw/magik/lint/Main.java
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,11 @@ public final class Main {
.build();
private static final Option OPTION_SHOW_CHECKS =
Option.builder().longOpt("show-checks").desc("Show checks and exit").build();
private static final Option OPTION_WITH_DEFAULT_VALUES =
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A better name would be --show-check-parameters-default-values or something.

Option.builder()
.longOpt("with-default-values")
.desc("Show default values when showing checks")
.build();
private static final Option OPTION_COLUMN_OFFSET =
Option.builder()
.longOpt("column-offset")
Expand All @@ -64,26 +69,28 @@ public final class Main {
.hasArg()
.type(PatternOptionBuilder.NUMBER_VALUE)
.build();
private static final Option OPTION_APPLY_FIXES =
Option.builder().longOpt("apply-fixes").desc("Apply fixes automatically").build();

private static final Option OPTION_DEBUG =
Option.builder().longOpt("debug").desc("Enable showing of debug information").build();
private static final Option OPTION_VERSION =
Option.builder().longOpt("version").desc("Show version and exit").build();
private static final Option OPTION_HELP =
Option.builder().longOpt("help").desc("Show this help and exit").build();
private static final Option OPTION_APPLY_FIXES =
Option.builder().longOpt("apply-fixes").desc("Apply fixes automatically").build();

static {
OPTIONS = new Options();
OPTIONS.addOption(OPTION_HELP);
OPTIONS.addOption(OPTION_MSG_TEMPLATE);
OPTIONS.addOption(OPTION_RCFILE);
OPTIONS.addOption(OPTION_SHOW_CHECKS);
OPTIONS.addOption(OPTION_WITH_DEFAULT_VALUES);
OPTIONS.addOption(OPTION_COLUMN_OFFSET);
OPTIONS.addOption(OPTION_MAX_INFRACTIONS);
OPTIONS.addOption(OPTION_APPLY_FIXES);
OPTIONS.addOption(OPTION_DEBUG);
OPTIONS.addOption(OPTION_VERSION);
OPTIONS.addOption(OPTION_APPLY_FIXES);
}

private static final Map<String, Integer> SEVERITY_EXIT_CODE_MAPPING =
Expand Down Expand Up @@ -175,6 +182,13 @@ public static void main(final String[] args)
Main.initDebugLogger();
}

if (commandLine.hasOption(OPTION_WITH_DEFAULT_VALUES)
&& !commandLine.hasOption(OPTION_SHOW_CHECKS)) {
final PrintStream errStream = Main.getErrStream();
errStream.println("--with-default-values can only be used with --show-checks");
System.exit(1);
}

if (commandLine.hasOption(OPTION_VERSION)) {
final String version = Main.class.getPackage().getImplementationVersion();
final PrintStream errStream = Main.getErrStream();
Expand All @@ -183,24 +197,7 @@ public static void main(final String[] args)
}

// Read configuration.
final MagikToolsProperties properties;
if (commandLine.hasOption(OPTION_RCFILE)) {
final File rcfile = (File) commandLine.getParsedOptionValue(OPTION_RCFILE);
final Path path = rcfile.toPath();
if (!Files.exists(path)) {
final PrintStream errStream = Main.getErrStream();
errStream.println("RC File does not exist: " + path);

System.exit(1);
}
properties = new MagikToolsProperties(path);
} else {
final Path currentWorkingPath = Path.of(".");
final Path path = ConfigurationLocator.locateConfiguration(currentWorkingPath);
properties =
path != null ? new MagikToolsProperties(path) : MagikToolsProperties.DEFAULT_PROPERTIES;
}

final MagikToolsProperties properties = Main.loadProperties(commandLine);
// Copy configuration from command line.
Main.copyOptionsToConfig(commandLine, properties);

Expand All @@ -210,8 +207,9 @@ public static void main(final String[] args)
final MagikLint lint = new MagikLint(properties, reporter);
final PrintStream outStream = Main.getOutStream();
final Writer writer = new PrintWriter(outStream);
lint.showEnabledChecks(writer);
lint.showDisabledChecks(writer);
final boolean showDefaultValues = commandLine.hasOption(OPTION_WITH_DEFAULT_VALUES);
lint.showEnabledChecks(writer, showDefaultValues);
lint.showDisabledChecks(writer, showDefaultValues);
writer.flush();
System.exit(0);
}
Expand Down Expand Up @@ -246,6 +244,27 @@ public static void main(final String[] args)
System.exit(exitCode);
}

private static MagikToolsProperties loadProperties(final CommandLine commandLine)
throws ParseException, IOException {
if (commandLine.hasOption(OPTION_RCFILE)) {
final File rcfile = (File) commandLine.getParsedOptionValue(OPTION_RCFILE);
final Path path = rcfile.toPath();
if (!Files.exists(path)) {
final PrintStream errStream = Main.getErrStream();
errStream.println("RC File does not exist: " + path);

System.exit(1);
}
return new MagikToolsProperties(path);
} else {
final Path currentWorkingPath = Path.of(".");
final Path path = ConfigurationLocator.locateConfiguration(currentWorkingPath);
return path != null
? new MagikToolsProperties(path)
: MagikToolsProperties.DEFAULT_PROPERTIES;
}
}

private static void copyOptionsToConfig(
final CommandLine commandLine, final MagikToolsProperties properties) {
if (commandLine.hasOption(OPTION_MAX_INFRACTIONS)) {
Expand Down
Loading
Loading