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

Tests for completions #186

Closed
snuyanzin opened this issue Nov 4, 2018 · 7 comments
Closed

Tests for completions #186

snuyanzin opened this issue Nov 4, 2018 · 7 comments

Comments

@snuyanzin
Copy link
Collaborator

Currently there is no tests for completions and it is hard to add anything new related to completions without breaking existing functionality

@julianhyde
Copy link
Owner

I reviewed PR #187:

  • SqlLineOpts.optionCompleters is a complex method. Can you add javadoc, and explain what it does and why it exists.
  • Can you change the name of property2possibleValues. It is too cute for my liking.
  • Is REGEX a hack?
  • What is BOOLEAN_NODE? Javadoc needed.
  • In some of your tests you are checking the first element of an iterator over a set. Are you assuming that the set contains just one element? If so, test that too. If not, your test is non-deterministic. Maybe you need a helper method to check the contents of sets.
  • I'm not a fan of the {{ trick of initializing collections by creating an anonymous sub-class. Can we make the initializations explicit? (I wish we could use Guava collections, which have initializers, but we can't.)

@snuyanzin
Copy link
Collaborator Author

snuyanzin commented Nov 8, 2018

Thank you for your review

  • SqlLineOpts.optionCompleters is a complex method. Can you add javadoc, and explain what it does and why it exists.

I added a javadoc for it. What it does is building of org.jline.builtins.Completers.RegexCompleter to satisfy the next conditions in decreasing order of priority

  1. If there are customizations via input Map<BuiltInProperty, Collection<String>> customCompletions then use it for completions
  2. If there are available values defined in sqlline.BuiltInProperty then use them for completions
  3. If the sqlline.SqlLineProperty.Type of property is Type.BOOLEAN then use sqlline.SqlLineProperty#BOOLEAN_VALUES for completion

If none of conditions is satisfied then there will no completions for values of that property. By the way now it works with colorschemes as well i.e. as mentioned in #164 (comment) after typing !set colorScheme and pressing tab it now suggests dark light vs2010 chester dracula obsidian solarized. The similar behavior is for !set outputFormat , !set isolation and setting any boolean property

  • Can you change the name of property2possibleValues. It is too cute for my liking.

renamed it to customCompletions

  • Is REGEX a hack?

removed it as a constant and left it inside a method only like "_value" to show that RegexCompleter should use it to match property value

  • What is BOOLEAN_NODE? Javadoc needed.

A set of values for boolean properties completions, renamed to BOOLEAN_VALUES

  • In some of your tests you are checking the first element of an iterator over a set. Are you assuming that the set contains just one element? If so, test that too. If not, your test is non-deterministic. Maybe you need a helper method to check the contents of sets.

added an additional check to check if the collection's size is one element

  • I'm not a fan of the {{ trick of initializing collections by creating an anonymous sub-class. Can we make the initializations explicit? (I wish we could use Guava collections, which have initializers, but we can't.)

Corrected it. By the way just could keep in mind that in Java 9 there is good api for that like

Map<Integer,String> map = Map.of(1, "A", 2, "B", 3, "C");

@julianhyde
Copy link
Owner

julianhyde commented Nov 19, 2018

When I run the tests, the tests all pass, but I get some nasty-looking stacks on stdout, e.g.

Running sqlline.CompletionTest
2018-11-19T14:10:00.222-0800  WARNING  Creating a dumb terminal
java.lang.IllegalStateException: Unable to create a system terminal
	at org.jline.terminal.TerminalBuilder.doBuild(TerminalBuilder.java:316)
	at org.jline.terminal.TerminalBuilder.build(TerminalBuilder.java:262)
	at sqlline.CompletionTest.getDummyLineReader(CompletionTest.java:155)
	at sqlline.CompletionTest.testResetPropertyCompletions(CompletionTest.java:69)

@julianhyde
Copy link
Owner

julianhyde commented Nov 19, 2018

By the way, I have rebased as 540faf5.

@snuyanzin
Copy link
Collaborator Author

Looks like jline3 warns about dumb terminal if the property is not set explicitly, so just made them being set while CompletionTest

@julianhyde
Copy link
Owner

Fixed in 6743244, PR #187.

@julianhyde
Copy link
Owner

I force-pushed another commit (including #223), so it's now fixed in 198447d.

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 a pull request may close this issue.

2 participants