Feature/urlparser improve3 pr1 #2641
Conversation
…to package org.postgresql.jdbcurlresolver
…nd fallback is default value. get() is added to return value without fallback (preparation for urlparser)
|
There are a few places where you are changing logLevel to loggerLevel, but we removed loggerLevel in 09c2c37 |
There is property LOGGER_LEVEL in PGProperty.java file: pgjdbc/pgjdbc/src/main/java/org/postgresql/PGProperty.java Lines 317 to 322 in 28c4da1 Property "logLevel" does not exist. While converting free-text string to enum property, the free-text value "logLevel" conversion fails. We need to change it either to correct value "loggerLevel" or remove invalid value at all. If LOGGER_LEVEL property is deprecated then it should be annotated accordingly? I have limited information about the development history. I can fix the faulty property. I have no basis for deciding that a correct property is wrong. This is where your specific input is required. |
|
Well you can see the comment right above the line that you referenced which says it's not used. |
|
@davecramer I removed usage of/references to "logLevel" and "loggerLevel". Is it better now? |
|
Yes, it's fine. I had to restart some checks. I will get around to merging it soon. |
| public @Nullable String get(Properties properties) { | ||
| public @Nullable String getOrDefault(Properties properties) { | ||
| return properties.getProperty(name, defaultValue); | ||
| } | ||
|
|
||
| /** | ||
| * Returns the value of the connection parameters according to the given {@code Properties} | ||
| * | ||
| * @param properties properties to take actual value from | ||
| * @return evaluated value for this connection parameter or null | ||
| */ | ||
| public @Nullable String get(Properties properties) { | ||
| return properties.getProperty(name); |
There was a problem hiding this comment.
This looks like a severe breakage of the public API.
Previously .get did take the default value into the consideration, while now get ignores the default value.
I believe code like PGProperty.PG_HOST.get(properties) is used in the wild, and it looks like it would change the behavior after this change.
There was a problem hiding this comment.
Alternatively we could just fix PGProperty.PG_HOST.get(properties) so that it worked as before ? Do you have any issues with the rest of the PR ?
There was a problem hiding this comment.
Well, I should have figured it out myself that get() could be used by external libraries.
Can I make rollback commit to the same PR here and it can be merged again? (or new PR is required)
What could be the name of another method:
- getWithoutDefault()
- getNoDefault()
- ?
There was a problem hiding this comment.
if the only issue with this PR is that get should be reverted we can just fix that as per PR # 2644
After contemplating this a bit, I'm not sure of the value of using getOrDefault ?
@vlsi do you have any other issues with the PR ?
There was a problem hiding this comment.
Well, we can probably keep getOrDefault so it explicitly conveys the behavior.
We can keep get() which would delegate to getOrDefault, and mark it as @Deprecated (without intention to remove the method), so the clients who use the methods could either migrate to the "new" getOrDefault or getWithoutDefault, etc.
I think getOrNull(...) would be better alternative than getWithoutDefault and getIgnoringDefault.
AFAIK, Properties (and Hashtable) does not support null values, and orNull in the method name gives an explicit warning that the method can return null.
I don't like getNoDefault.
Yet another alternative could be getIgnoringDefault, however, I think getOrNull is better for "get value or return null if it is missing in the properties".
There was a problem hiding this comment.
if the only issue with this PR is that get should be reverted we can just fix that as per PR #2644
yes, that's good
I think
getOrNullis better ...
I like it.
This reverts commit ee06e22.
All Submissions:
This PR is replacement for PR #2569
This is first PR. This PR does not change any functionality.