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

Have DataGrip configure it using "Advanced Settings" #3937

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

QiE2035
Copy link

@QiE2035 QiE2035 commented Dec 1, 2023

[Bing translated from Chinese]

Add some DriverPropertyInfo to getPropertyInfo so that DataGrip (IntelliJ IDEA, CLion, etc) can be configured with "Advanced Settings".

I'll admit that this implementation is really bad, but at least now we have an "Advanced Settings" with some content.

image

Copy link
Contributor

@katzyn katzyn left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution!

@@ -0,0 +1,78 @@
package org.h2.util;
Copy link
Contributor

Choose a reason for hiding this comment

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

This class should be in org.h2.engine package to avoid interaction with actual configuration classes.

newProp("RECOMPILE_ALWAYS", false, "Database setting RECOMPILE_ALWAYS (default: false). \nAlways recompile prepared statements."),
newProp("REUSE_SPACE", true, "Database setting REUSE_SPACE (default: true). \nIf disabled, all changes are appended to the database file, and existing content is never overwritten. \nThis setting has no effect if the database is already open."),
newProp("SHARE_LINKED_CONNECTIONS", true, "Database setting SHARE_LINKED_CONNECTIONS (default: true). \nLinked connections should be shared, that means connections to the same database should be used for all linked tables that connect to the same database."),
newProp("ZERO_BASED_ENUMS", false, "Database setting ZERO_BASED_ENUMS (default: false). \nIf set, ENUM ordinal values are 0-based."),
Copy link
Contributor

Choose a reason for hiding this comment

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

  • There is no MV_STORE setting.
  • ZERO_BASED_ENUMS is going to be removed, don't list it here too.
  • I think it's a bad idea to hard-code defaults here, it will be better to grab actual defaults from DbSettings or other sources.
  • Many settings here (OPTIMIZE_* and possibly others) are just kill switches. If some bug in optimization will be found, such switch can be used as a temporary workaround until this bug will be fixed. I think it is a bad idea to return them here, because they can be misused by accident.
  • DEFAULT_TABLE_ENGINE shouldn't be listed too, it exists for exotic use cases in third-party solutions.

I think it will be better to list only useful settings here, including some settings from other sources, take a look on ConnectionInfo, it contains names of various other settings, including popular ones. @grandinj, what do you think?

You also need to break long lines, our code formatting rules don't allow lines longer than 120 characters.

@@ -117,7 +118,7 @@ public int getMinorVersion() {
*/
@Override
public DriverPropertyInfo[] getPropertyInfo(String url, Properties info) {
return new DriverPropertyInfo[0];
return DriverPropertyInfoSet.get();
Copy link
Contributor

Choose a reason for hiding this comment

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

TestMetaData needs to be updated.

final String name,
final String value,
final String desc,
final String... choice) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Usually we don't mark method parameters and local variables as final.

@QiE2035
Copy link
Author

QiE2035 commented Dec 1, 2023

Ok, so do I need to modify it right away? I seem to see you initiating a discussion with another person in the comments, do you need me to wait for the outcome of your discussion?

@katzyn
Copy link
Contributor

katzyn commented Dec 1, 2023

Which settings we want to announce is an open question, but you can try to resolve other problems right now.

1. Move to `org.h2.engine`
2. Rename to `DriverPropertyInfoProvider`
3. Break long lines
4. Comment unnecessary property
5. Use `DbSettings.DEFAULT` as default values
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.

None yet

2 participants