Implemented new cli, based on a different framework

[TwoOfTwelve]

Implementation for language specific cli arguments. Also contains a rewrite for the cli module using a different, still maintained library.

Adresses #920, #591 and #550.

The new cli should support all options, that the previous version had. Due to the cli being implemented using a new library, it might behave a little different.

The language can now be defined as a subcommand ('jplag java' instead of jplag -l java'). This allows language specific parameters to be set.

Every option now has a long name.

Short options with more than one character (-new or -bc) are still supported, but should be discouraged as this is not intuitive.

[Kr0nox]

Just three missing Documentations for public methods.

In addition tho: You removed the constants (enum in this case) for the for the CLI parameters. I would put to discussing keeping them in some kind of way, so that they can be reused in the tests (where now they are also just strings) and so in case they may be required somewhere in the future they are uniform across JPlag

[TwoOfTwelve]

You are right about the missing documentation, I will add it.

In regards to the missing constants, I would argue, that the tests using constants defined in the cli kind of defeats their purpose, since changing the names of parameters would be a change of the public interface and thus should be reported by the tests.
Also I think, that no application code should refer to arguments by their names and instead just use the variables.

It might however be useful to move the string into constants withing the test source, so they can be changed at a central point for all tests.

Also the tests for the cli module are rather sparse currently, so maybe we should create a separate issue to make them more useful.

[Kr0nox]

I agree with your point about refering to the variables instead of the cli argument.

Also the tests for the cli module are rather sparse currently, so maybe we should create a separate issue to make them more useful.

#179

[tsaglam]

A few things to consider here...

[TwoOfTwelve]

I implemented most of your suggestions @tsaglam, except those I commented on and those concerning tests, as I want to hear your opinion on the whole enum/no enum matter first.

Once again, this solution is not perfect. But using java enums we remove any type safety when accessing the options, this way the not type safe code is at least at a central point in picocli or in the cli module, concerning the language specific options.

Also, yes we could use an enum or constants to store all option names, but I would prefer them to be as close as possible to the options definition.

[tsaglam]

I implemented most of your suggestions, except those I commented on and those concerning tests, as I want to hear your opinion on the whole enum/no enum matter first.

see my answer above, we can always change it if we want to go the enum route, as this is not public API.

[tsaglam]

@TwoOfTwelve, we probably need to document the changed/additional CLI parameters in the readme + wiki docs, right?
Also, could you extend the PR description regarding these changes for the user? Makes it easier when writing the release notes.

[TwoOfTwelve]

I changed the description and the README. I don't know that much about GitHub. Can I update the wiki for a specific branch or should I just update the general Wiki?

[tsaglam]

We have a specific action that builds the wiki from the docs folder (since #1017). So just modify the docs in the branch of this PR.

[Kr0nox]

A few inconsistencies and possible missunderstandings I found

[tsaglam]

Just a few minor things.

[Kr0nox]

2 small things I found

[Kr0nox]

And in line 19 of the README it says:

line using the -l <cli argument name>

Should this be changed or at least mention the subcommand system?

[TwoOfTwelve]

I changed the indentation as we discussed. I also found a way to change it for the cli so it is consistent with the documentation.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

54.4% Coverage
0.0% Duplication

[brodmo]

@TwoOfTwelve I don't really understand how to use this, could you explain? How do I recover a language-specific option from a JPlagOptions object? How do I pass a language-specific option in the first place, is it any different from an option that is not language-specific? Can I set flags instead of using key-value pairs?

[TwoOfTwelve]

Let's say for example you want to add an option to limit the number of tokens the java language extracts from a single file (I know this does not make sense, but just as an example).

You create a class for the java options looking like this:

public class JavaOptions extends LanguageOptions {
    LanguageOption<Integer> maxTokens = createDefaultOption(OptionType.integer(), "maximumFileTokens", 0);
}

You create an instance of it in the java language class and publish it through the getOptions method like so:

    @Override
    public JavaOptions getOptions() {
        return this.options;
    }

You can then pass the instance to the parser when "parse" is called. The parser can then use:

options.maxTokens.getValue()

to obtain the options value an use that in the parsing process.

To set the option from the cli you don't have to write any more code. With the given code you could, for example, set the option like this:

jplag java --maximumFileTokens 10 <sumission directory>

To set it from the api you need to make the following call before calling the JPlag interface:

Language lang = new Language(); //referring to the java language here
lang.getOptions().maxTokens.setValue(4);
//now call jplag with lang as your language

The ways you can set options through the cli depends mostly on picocli. If you would configure an option to be a boolean picocli should allow you to set it as a switch. Since I added only integer and string as OptionTypes for now, you would need to expand the OptionTypes class to include Boolean. For all primitive types or lists you only need to copy the structure from integer or string. For more complex types you need to expand the cli and map the type for picocli.

I hope this helps, if not I will be glad to explain it to you in more detail.

[brodmo]

So the language options are completely separate from the JPlagOptions? Why?

Language lang = new Language(); //referring to the java language here
lang.getOptions().maxTokens.setValue(4);
//now call jplag with lang as your language

Doesn't the typing prevent this? lang.getOptions() has type LanguageOptions, which doesn't have a maxTokens field.

Is there a way to have shared language options? I want to implement a normalization option which many languages could have in the future.

[TwoOfTwelve]

The option are separate, because joining them into the JPlag options would have meant a lot of cascading changes through the core module. Besides that I tried to only change the API as little as necessary.

Typing does not prevent this. It is called variance. A subclass can always use more specific return types than its super class. Of course once you cast your language to the generic Language interface you loose that information. However once you do that you should not need the specific type and options any longer. For "automatic" use there is a method that returns a list of all options.

Currently the only way to implement shared options would be inheritance, but I would not suggest that. I could add a way to use shared options, but if it is something that affects all or most languages we should consider making it a global option instead. That, however, is a design choice an should ultimately be made by a @jplag/maintainer.

[brodmo]

For my use case, I've added a getOptionsAsMap method:

/**
  * @return all options, mapped by their name
  */
 public Map<String, LanguageOption<?>> getOptionsAsMap() {
     return options.stream().collect(Collectors.toUnmodifiableMap(LanguageOption::getName, o -> o));
 }

which I then use like this:

if (Boolean.TRUE.equals(options.language().getOptions().getOptionsAsMap().get("normalize").getValue()))
    submissions.forEach(Submission::normalize);

but this isn't really a great solution because I have to get the option by name and the typing is completely lost. Anyways, thanks for your help.

Also FYI -h doesn't show all possible flags with the language subcommands, I'm not sure this is intentional:

[TwoOfTwelve]

I already commented to most of this in your pr. What do you think is missing? It does show your normalization flag and the two normal help and version flags.

[brodmo]

But I can still use all of the other flags, no? To me this is confusing but I guess it is intended behavior then, in which case nevermind

[TwoOfTwelve]

"java" here is a subcommand. The full syntax is jplay [jplag options] java [java options]. The jplag options are shown by "jplag -h" and the java options are shown by "jplag java -h".