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

Added the option to specify a result file with --move VIEW #2075

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

TwoOfTwelve
Copy link
Contributor

@TwoOfTwelve TwoOfTwelve commented Nov 20, 2024

Added the option to specify a result file with --mode VIEW

Allows to specify exactly one file with either the positional argument, --new, --old or -r

The documentation has been adapted in the CLI and in the README

@tsaglam tsaglam added enhancement Issue/PR that involves features, improvements and other changes minor Minor issue/feature/contribution/change labels Nov 22, 2024
@tsaglam tsaglam added this to the 6.0.0 milestone Nov 22, 2024
Copy link

Copy link
Member

@tsaglam tsaglam left a comment

Choose a reason for hiding this comment

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

Just a few minor issues (+ Sonar issues).

Comment on lines +56 to +57
@Option(names = {"-M",
"--mode"}, description = "The mode of JPlag. If VIEW is chosen, you can specify a result file to display with either the positional argument, '--new', '--old' or '-r'. Make sure to only specify one if you do that. One of: ${COMPLETION-CANDIDATES} (default: ${DEFAULT_VALUE})")
Copy link
Member

Choose a reason for hiding this comment

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

I was wondering whether we should simplify this and not tell the users that there are multiple options. Just document one, but leave the functionality for all others in.

@@ -29,6 +30,7 @@
public class CliInputHandler {
private static final String OPTION_LIST_HEADING = "Parameter descriptions: ";

private static final String AMBIGUOUS_VIEW_FILE = "There are multiple files selected for '--mode VIEW', Please make sure to only specify one.";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
private static final String AMBIGUOUS_VIEW_FILE = "There are multiple files selected for '--mode VIEW', Please make sure to only specify one.";
private static final String AMBIGUOUS_VIEW_FILE = "There are multiple files specified for '--mode VIEW', please make sure only to specify one.";

validOptions.addAll(List.of(this.options.newDirectories));
validOptions.addAll(List.of(this.options.oldDirectories));

if (this.parseResult.hasMatchedOption('r')) {
Copy link
Member

Choose a reason for hiding this comment

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

Magic character. We cannot access the CliOption itself as it is an annotation, right? Then maybe just a constant here.

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's possible, but really ugly. I'd prefer the constant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Issue/PR that involves features, improvements and other changes minor Minor issue/feature/contribution/change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants