Skip to content

Unhelpful error message when a required parameter is not configured #161

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

Merged
merged 13 commits into from
Mar 21, 2025

Conversation

fmeheust
Copy link
Member

Changed parameter parser implementation so that all parameters are added to the parameter set even those for which no value has been set. This allows to get the parameter name to give helpful messages when a parameter is missing.

@fmeheust fmeheust self-assigned this Mar 19, 2025
@fmeheust fmeheust linked an issue Mar 19, 2025 that may be closed by this pull request
@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Mar 19, 2025
@fmeheust fmeheust requested a review from jeandelavarene March 19, 2025 10:06
Copy link
Member

@Michael-A-McMahon Michael-A-McMahon left a comment

Choose a reason for hiding this comment

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

I like the removal of ParameterParser.defaultValueSetter, and the cascading removal of code around that field. Less code is always good!
Please see my comments before merging these changes.

@@ -116,7 +129,6 @@ public Builder addParameter(
String name, Parameter<String> parameter, String defaultValue) {
addParameterParser(
name,
(builder) -> builder.add(name, parameter, defaultValue),
Copy link
Member

Choose a reason for hiding this comment

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

It seems like we lose the defaultValue here; Maybe we want the valueSetter function as:
(value, builder) -> builder.add(name, parameter, value == null ? defaultValue : value)

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for catching it!

@@ -212,52 +212,5 @@ <T> Builder addParameter(
Builder addParameter(
Copy link
Member

Choose a reason for hiding this comment

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

In the JavaDoc above, let's add update it to specify that a valueSetter function must be able to handle a null input. I've added in the 2nd paragraph below, and left everything else unchanged.

/**
     * <p>
     * Adds a parameter that a parser identifies by a given
     * {@code name}, and invokes a {@code valueSetter} function for assigning
     * the value. The inputs to the {@code valueSetter} function are the value
     * as it appears in text form and a {@link ParameterSetBuilder} that builds
     * the parsed {@link ParameterSet}. If the name passed to this method is "x", and
     * the parsed input includes an assignment such as "x=0", then "0" is input
     * to the {@code valueSetter}.
     * </p><p>
     * The {@code valueSetter} must be able to handle a null input. A null input will occur
     * when no value is set for the parameter. The {@code valueSetter} may simply pass
     * the null value onto the {@code ParameterSetBuilder}, or it may pass in a default
     * value.
     * </p><p>
     * This method is designed for cases where a single parameter in text format
     * may map to multiple {@link Parameter} objects. The {@code valueSetter}
     * function can perform multiple calls to set each parameter, as in this
     * example:
     * </p>
     * <pre>{@code
     * builder.addParameter("coordinate", (value, parameterSetBuilder) -> {
     *   // Split "x,y,z" formatted value
     *   String[] xyz = value.split(",");
     *   parameterSetBuilder.add("coordinate", X, xyz[0]);
     *   parameterSetBuilder.add("coordinate", Y, xyz[1]);
     *   parameterSetBuilder.add("coordinate", Z, xyz[2]);
     * });
     * }</pre>
     *
     * @param name Name of the parsed parameter. Not null.
     * @param valueSetter Parses and sets the value of parameter(s) from text
     *        input. Not null.
     * @return This builder.
     */

@@ -52,7 +52,9 @@ final class ParameterSetBuilderImpl implements ParameterSetBuilder {

@Override
public <T> ParameterSetBuilder add(String name, Parameter<T> parameter, T value) {
Copy link
Member

Choose a reason for hiding this comment

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

In the interface class where this method is defined, ParameterSetBuilder.java, we can update the JavaDoc:

  /**
   * Adds a parameter with a given name and value. If the value is null, the ParameterSet will
   * behave as if no value has been set for the parameter. Calling this method with a null value
   * allows the ParameterSet to create error messages that identify the {@code name} of 
   * missing parameters.
   *
   * @param <T> The type of value that is assigned to the parameter
   * @param name The parameter name. Not null.
   * @param parameter The parameter that is assigned with the {@code name} and
   *                  {@code value} in this set. Not null.
   * @param value The value, or null if the parameter is not configured with a value.
   * @return A reference to this object
   */

@jeandelavarene jeandelavarene merged commit 0a830a7 into main Mar 21, 2025
2 checks passed
@jeandelavarene jeandelavarene deleted the missing-parameter branch March 21, 2025 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OCA Verified All contributors have signed the Oracle Contributor Agreement.
Projects
None yet
3 participants