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

[LO extension / Standalone] add the possibility to define more than o… #10542

Merged
merged 3 commits into from Apr 29, 2024

Conversation

FredKruse
Copy link
Contributor

The change creates the ability to define more than one custom option per rule.
All existing rules with custom options have been adjusted. The options panel has been expanded accordingly.
The API and the HTTP server (ConfigurationInfo) have been adapted. Upward and downward compatibility between versions lower than 6.5 and higher than 6.4 was ensured.
A second option was added for the GermanFillerWordsRule as a test case.
All changes were tested.
To-do: Some more rules should be expanded to include additional options.
A merge can be performed.

…ne user defined option to a rule by option panel
String str = values[i].trim();
char c = values[i].charAt(0);
str = str.substring(1);
if (c == 's') {
Copy link
Member

Choose a reason for hiding this comment

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

A comment with examples would be nice here

Comment on lines +77 to +80




Copy link
Member

Choose a reason for hiding this comment

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

empty lines

char c;
for (int i = 0; i < obs.length; i++) {
Object o = obs[i];
if (o instanceof Integer) {
Copy link
Member

Choose a reason for hiding this comment

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

Is this code duplicated from above? Can we avoid the duplication?

Object maxConfigurableValue;
if (defaultType.equals("Integer")) {
defaultValue = (int) rOptions.get(i).get("defaultValue");
minConfigurableValue = (int) rOptions.get(i).get("minConfigurableValue");
Copy link
Member

Choose a reason for hiding this comment

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

minConfigurableValue etc. should probably be a constant, as it is used several times

@SuppressWarnings("unchecked")
List<Map<String,Object>> rOptions = (List<Map<String, Object>>) o;
ruleOptions = new RuleOption[rOptions.size()];
for (int i = 0; i < rOptions.size(); i++) {
Copy link
Member

Choose a reason for hiding this comment

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

This code is duplicated?

Comment on lines 745 to 762
Object[] objects = new Object[values.length];
for (int i = 0; i < values.length; i++) {
char c = values[i].charAt(0);
String str = values[i].substring(1);
if (c == 's') {
objects[i] = str;
} else if (c == 'b') {
objects[i] = Boolean.parseBoolean(str);
} else if (c == 'f') {
objects[i] = Float.parseFloat(str);
} else if (c == 'd') {
objects[i] = Double.parseDouble(str);
} else if (c == 'c') {
objects[i] = str.charAt(0);
} else if (c == 'i') {
objects[i] = Integer.parseInt(str);
} else { // compatible to old version
objects[i] = Integer.parseInt(values[i]);
Copy link
Member

Choose a reason for hiding this comment

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

This is duplicated?

@danielnaber
Copy link
Member

@fabrichter Maybe you can quickly check whether the change in UserConfig is harmless?

@FredKruse
Copy link
Contributor Author

The requested changes was done

@fabrichter
Copy link
Collaborator

fabrichter commented Apr 26, 2024

@danielnaber
For UserConfig, the change to an Array probably breaks .equals and thus pipeline caching because equals on arrays checks for reference equality. But this most likely isn't relevant in practice, since for the use case where pipeline caching is enabled, configurableRuleValues is always empty. So I think we don't need to change this, but a comment about it in .equals would be good to add.

I would also add a better getter for the configuration values like this:

  public <T> T getConfigValueByID(String ruleID, int index, Class<T> clazz, T defaultValue) {
    Object[] value = configurableRuleValues.get(ruleID);
    if (value == null) return defaultValue;

    if (index >= value.length) return defaultValue;
    if (!clazz.isInstance(value[index])) return defaultValue;

    return (T) value[index];
  }

This should be used instead of the existing manual accesses and checks to inner values of configurableRuleValues, as the checks are repetitive and not done consistently as-is.

@FredKruse
Copy link
Contributor Author

FredKruse commented Apr 27, 2024

The requested changes from @fabrichter and @danielnaber were done.

@danielnaber danielnaber merged commit b8ec9bb into master Apr 29, 2024
2 checks passed
@danielnaber danielnaber deleted the multi-options-rules branch April 29, 2024 08:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants