On Fri, 15 Mar 2024 13:04:00 GMT, Sean Coffey <[email protected]> wrote:
>> Weijun Wang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Mark's comments
>
> src/java.base/share/classes/sun/security/util/Debug.java line 172:
>
>> 170: * settings. For example,
>> 171: * {@snippet lang=java:
>> 172: * Map<String,String> settings = loadLoginSettings();
>
> minor nit - needs a space : `Map<String, String>`
Will add one.
> src/java.base/share/classes/sun/security/util/Debug.java line 174:
>
>> 172: * Map<String,String> settings = loadLoginSettings();
>> 173: * String property = settings.get("login");
>> 174: * Debug debug = Debug.of("login", setting);
>
> did you mean to use the `property` variable name here ?
Ah, yes.
> src/java.base/share/classes/sun/security/util/Debug.java line 180:
>
>> 178: * @return a new Debug object if the property is true
>> 179: */
>> 180: public static Debug of(String option, String property) {
>
> the `property` name is a bit confusing here IMO. Most use cases will be
> string corresponding to a boolean (or null) - Would a boolean paramater make
> more sense ? Otherwise. I'd suggest renaming variable to something like
> `debugEnabled`
I was thinking that it will contain more options like `true+timestamp` later.
By that time, we only need to update the `Debug` class to add this feature.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/18199#discussion_r1526297798
PR Review Comment: https://git.openjdk.org/jdk/pull/18199#discussion_r1526297546
PR Review Comment: https://git.openjdk.org/jdk/pull/18199#discussion_r1526299579
