8000 Have DataGrip configure it using "Advanced Settings" by QiE2035 · Pull Request #3937 · h2database/h2database · GitHub
[go: up one dir, main page]

Skip to content

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 reaso 8000 n 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.

@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 8000
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.

2 participants

0