Skip to content

Conversation

ruslanys
Copy link
Contributor

--list option should be displayed by default out-of-the-box. But InitCommand had 2 options with -l key: -l for --language and -l for --list. In that case, when OptionHelpFormatter.format(...) pushing options into TreeSet, the --list option disappears.

@pivotal-issuemaster
Copy link

@ruslanys Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-issuemaster
Copy link

@ruslanys Thank you for signing the Contributor License Agreement!

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Sep 13, 2018
@ruslanys
Copy link
Contributor Author

Fix #13940

@philwebb philwebb added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Sep 13, 2018
@philwebb philwebb changed the title Fix #13940 Include --list in the tab completion for spring init from CLI Sep 13, 2018
@philwebb philwebb changed the title Include --list in the tab completion for spring init from CLI Include --list in the tab completion for CLI init commmand Sep 13, 2018
@philwebb philwebb added the type: bug A general bug label Sep 13, 2018
@philwebb philwebb added this to the 2.0.x milestone Sep 13, 2018
@philwebb philwebb added for: merge-with-amendments Needs some changes when we merge and removed type: enhancement A general enhancement labels Sep 13, 2018
@philwebb
Copy link
Member

Very interesting that the duplicate causes it to just disappear. I wonder if we can force the app to fail hard when that happens?

@ruslanys
Copy link
Contributor Author

@philwebb I agree this is strange behavior.
Unfortunately, in the CLI context, it's not clear when can we check options uniqueness.
In the current implementation, Spring Boot CLI is initializing command options when a command choose.
That means that we will be able to throw an exception only when user types spring init.

Moreover, for options recognizing Spring Boot CLI is using joptsimple library, especially OptionParser.

So, one of the ideas is to update the getParser() method and put validation under options() invocation.
Like:

private void checkOptionsUniqueness() {
	List<String> keys = parser.recognizedOptions().values().stream()
			.distinct()
			.flatMap(optionSpec -> optionSpec.options().stream())
			.collect(Collectors.toList());

	Set<String> hashSet = new HashSet<>();
	for (String key : keys) {
		if (!hashSet.add(key)) {
			throw new IllegalStateException("Option key [" + key + "] is duplicated.");
		}
	}
}

I can provide a separate PR for this.

@ruslanys
Copy link
Contributor Author

Unfortunately, the example above won't fix all problems.
If somebody put several exactly the same keys, for instance, "list" for several options, then OptionsParser won't provide us with the actual list of options and parser.recognizedOptions() will return only one of the options.

@ruslanys
Copy link
Contributor Author

The second approach is to validate option name and aliases at OptionHandler.option(...) method, but it will be possible to get parser directly and register option out of the OptionHandler class.

@ruslanys
Copy link
Contributor Author

@philwebb Please take a look at #14466.

@snicoll snicoll changed the title Include --list in the tab completion for CLI init commmand Remove duplicate -l option for init command Oct 9, 2018
@snicoll snicoll removed the for: merge-with-amendments Needs some changes when we merge label Oct 9, 2018
@snicoll snicoll self-assigned this Oct 9, 2018
@snicoll snicoll modified the milestones: 2.0.x, 2.0.6 Oct 9, 2018
snicoll pushed a commit that referenced this pull request Oct 9, 2018
snicoll added a commit that referenced this pull request Oct 9, 2018
* pr/14460:
  Polish "Remove duplicate -l option for init command"
  Remove duplicate -l option for init command
@snicoll
Copy link
Member

snicoll commented Oct 9, 2018

Closed by 6e6c22c

@snicoll snicoll closed this Oct 9, 2018
@snicoll
Copy link
Member

snicoll commented Oct 9, 2018

@ruslanys thank you very much for making your first contribution to Spring Boot.

@ruslanys ruslanys deleted the 2.0.x branch October 9, 2018 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug A general bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants