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

#428 auto complete list #3980

Merged
merged 40 commits into from Feb 18, 2024

Conversation

manticore-projects
Copy link
Contributor

@manticore-projects manticore-projects commented Jan 24, 2024

Fixes issues with the Auto Completion when quotes schemas or table names are involved.
Added some more test cases for quoted schemas and quoted table names.

I have not been able to fix the Web Client inserting the selection into the SQL (which is broken for quoted items).

Partially fixes #428

@@ -78,12 +78,18 @@ public boolean autoComplete(Sentence sentence) {
DbSchema bestSchema = null;
for (DbSchema schema: schemas) {
String name = StringUtils.toUpperEnglish(schema.name);
String quotedName = "\"" + schema.quotedName + "\"";
Copy link
Contributor

Choose a reason for hiding this comment

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

schema.quotedName can be either quoted or unquoted, you shouldn't try to quote it again.

It is actually a normalized name (how it should be inserted into SQL), so you can rename this field and introduce a new one with a real always quoted name (constructed with StringUtils.quoteIdentifier()).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you very much for feedback and advice.
I have fixed that and also added tests accordingly for identifiers like "quoted schema" and "quoted tablename".

@katzyn
Copy link
Contributor

katzyn commented Jan 25, 2024

I think these historic conversions to upper case don't look right, especially for some Unicode characters. String.regionMatches(false, …) should be used instead, some helper method in StringUtils can be introduced (startsWithIgnoreCase(String, String) or something like it).

@manticore-projects
Copy link
Contributor Author

I think these historic conversions to upper case don't look right, especially for some Unicode characters. String.regionMatches(false, …) should be used instead, some helper method in StringUtils can be introduced (startsWithIgnoreCase(String, String) or something like it).

Agreed, I just wanted to avoid changing too much.
I will introduce a StringUtils.startsWithIgnoringCase(...), thanks for the recommendation.

@manticore-projects
Copy link
Contributor Author

I think these historic conversions to upper case don't look right, especially for some Unicode characters. String.regionMatches(false, …) should be used instead, some helper method in StringUtils can be introduced (startsWithIgnoreCase(String, String) or something like it).

Remove all toUpperCase() calls and implemented a StringUtils.startsWithIgnoringCase() dealing with all Unicode shenanigans.

@manticore-projects
Copy link
Contributor Author

Totally unrelated, please allow me to ask: why are those test cases implemented in a special way but not using regular Junit5? Of course it does not matter in general although I experiences some disadvantages:

  • no Parametrized test (e.g. as per statement)
  • I can't call a test method directly, but have to fiddle with the main(String[] args) method
  • IDE can't benefit from Coverage and Test reporting

No need to rock the boat, just want to understand the rational behind.

@katzyn
Copy link
Contributor

katzyn commented Feb 1, 2024

  1. Both Java and JUnit were significantly less advanced when these tests were written.
  2. H2 is mostly tested with integration tests and unit-test frameworks aren't fully suitable for them.
  3. Our tests are executed multiple times in different configurations (in-memory, networked, etc). It wasn't simple to achieve this with JUnit, but maybe its modern versions are better, I don't know.

@manticore-projects
Copy link
Contributor Author

manticore-projects commented Feb 13, 2024

I have fixed the String Comparison and am using Collator now. Sorry for the delay.
Please reconsider merging it.

PS: please why do the Unit Tests fail? I don't think this was related to my PR.

@manticore-projects
Copy link
Contributor Author

Team, any objections on merging this?

@katzyn katzyn merged commit 33da479 into h2database:master Feb 18, 2024
4 checks passed
@manticore-projects
Copy link
Contributor Author

Thanks a lot!

catull pushed a commit to catull/h2database that referenced this pull request Mar 9, 2024
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.

autocomplete with quoted schemas does not complete
3 participants