-
Notifications
You must be signed in to change notification settings - Fork 6.1k
8360575: java.util.Properties.list() methods trim each value to 37 characters in the listed output #26018
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
base: master
Are you sure you want to change the base?
Conversation
…aracters in the listed output
👋 Welcome back jpai! A progress list of the required criteria for merging this PR into |
❗ This change is not yet ready to be integrated. |
Webrevs
|
Are you sure you want to describe the format, even in an implNote? Although the string representation probably hasn't changed in 25+ years, it's not something that anything should become dependent on. Updating the method description to include an example of how to print out the properties and their values might be more useful. We also need to think about whether to just deprecate this method (ThreadGroup.list too). |
I wasn't too sure how much detail to include. I've now trimmed it down to just note that the values are trimmed to 37 characters.
I've updated the PR to include a snippet which shows an alternate way of listing the
Would you suggest doing it as part of the current changes or as a separate task? I can create follow up issues if it's OK to do that separately. |
History note from the commit added by avh: "Added some debugging methods. jdk/src/share/classes/java/util/Properties.java 1.8 Tue 13-06-1995 18:04:34" It does not seem to be intended as anything but a debugging aid. |
* p.forEach((k, v) -> out.println(k + "=" + v)); | ||
* } | ||
* | ||
* @param out a PrintStream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: (here and L1263) "the output PrintStream"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
* this method writes only the first 37 characters of that value | ||
* followed by 3 dot characters. | ||
* <p> | ||
* An alternative for listing the {@code Properties} to a {@code PrintStream} is: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I've now updated the PR with this text.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not a JDK reviewer, but it looks good to me. Thank you!
You'll need a CSR since you're changing spec text. (output stream -> PrintStream) and the new ImplNote. Consider proposing better replacement. But that's a bigger (possibly more satisfying task) |
isn't this PR a mere clarification, as opposed to the spec change? Or any change is a spec change in the JDK project? (openjfx allows for minor javadoc corrections without a CSR) |
For JDK, (nearly) all changes to the javadoc require a CSR, if the changes the published form. |
* @param out an output stream. | ||
* @implNote If any property's value is greater than 40 characters then | ||
* this method writes only the first 37 characters of that value | ||
* followed by 3 dot characters. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the note needs to that specific. I think it need only say that the implementation truncates the output for very long property values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello Alan, I think not noting the character count which determines the truncation would lead to questions about what "very long property values" are. However, this is a method which is specified to be for debug purposes only and I see that there's a suggestion to deprecate these methods. So I have gone ahead and updated this text to follow your suggestion. I'll respond to the deprecation proposal separately in this PR.
I see that both Alan and Roger suggest that we consider deprecating these methods (and the one in |
/csr needed |
@jaikiran has indicated that a compatibility and specification (CSR) request is needed for this pull request. @jaikiran please create a CSR request for issue JDK-8360575 with the correct fix version. This pull request cannot be integrated until the CSR request is approved. |
* Properties p = ... | ||
* PrintWriter out = ... | ||
* // list the properties to PrintWriter | ||
* p.forEach((k, v) -> out.println(k + "=" + v)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should our example recommend using toString
as a utility to obtain a debug string instead, as it performs no truncation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using toString()
will be harder to read; as proposed each property is on a separate line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
* Properties p = ... | ||
* PrintWriter out = ... | ||
* // list the properties to PrintWriter | ||
* p.forEach((k, v) -> out.println(k + "=" + v)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using toString()
will be harder to read; as proposed each property is on a separate line.
If I understand the code, Properties::forEach doesn't walk up the default Properties. Properties::list walks the defaults. To get the same keys you would have to use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The example should be updated as suggested.
The example could get complicated; depending on what output is desired. The existence of a separate "default" Properties object adds complexity. |
Can I please get a review of this doc-only change which proposes to clarify the current implementation of the
java.util.Properties.list(...)
methods?As noted in https://bugs.openjdk.org/browse/JDK-8360575, the current implementation trims each value to a size of 37 when printing out the value. This behaviour isn't documented by these methods. The change in this PR adds an
@implNote
to make a mention of this current behaviour.Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/26018/head:pull/26018
$ git checkout pull/26018
Update a local copy of the PR:
$ git checkout pull/26018
$ git pull https://git.openjdk.org/jdk.git pull/26018/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 26018
View PR using the GUI difftool:
$ git pr show -t 26018
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/26018.diff
Using Webrev
Link to Webrev Comment