RFR (JDK10) 8183743: Umbrella: add overloads that take a Charset parameter

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR (JDK10) 8183743: Umbrella: add overloads that take a Charset parameter

Joe Wang
Hi,

Adding convenient methods that take a Charset to several classes that
have already had methods with a Charset/Encoding name as a parameter. To
avoid any impact on compatibility and JCK tests, we've kept the existing
methods virtually untouched except making a reference to the new ones to
encourage the use of these new methods going forward. The javadocs of
the new methods however, may be more complete with details on handling
edge cases / Exceptions.

JBS: https://bugs.openjdk.java.net/browse/JDK-8183743
webrev: http://cr.openjdk.java.net/~joehw/jdk10/8183743/webrev/index.html

Thanks,
Joe

Reply | Threaded
Open this post in threaded view
|

Re: RFR (JDK10) 8183743: Umbrella: add overloads that take a Charset parameter

Alan Bateman


On 01/12/2017 06:11, Joe Wang wrote:

> Hi,
>
> Adding convenient methods that take a Charset to several classes that
> have already had methods with a Charset/Encoding name as a parameter.
> To avoid any impact on compatibility and JCK tests, we've kept the
> existing methods virtually untouched except making a reference to the
> new ones to encourage the use of these new methods going forward. The
> javadocs of the new methods however, may be more complete with details
> on handling edge cases / Exceptions.
>
> JBS: https://bugs.openjdk.java.net/browse/JDK-8183743
> webrev: http://cr.openjdk.java.net/~joehw/jdk10/8183743/webrev/index.html
I looked through the javadoc for the updated/new methods and it mostly
looks good.

I think URLDecoder.decode methods should have @throws
IllegalArgumentException. I realize it's implementation specific as to
whether IAE is thrown with bad input but given that the RI does throw
IAE then consumers of the API should be prepared for it. The @implNote
can stay and probably should be copied into the legacy decode method too.

One of the new constructors on Scanner needs @since 10.

-Alan
Reply | Threaded
Open this post in threaded view
|

Re: RFR (JDK10) 8183743: Umbrella: add overloads that take a Charset parameter

Joe Wang


On 12/1/17, 3:16 AM, Alan Bateman wrote:

>
>
> On 01/12/2017 06:11, Joe Wang wrote:
>> Hi,
>>
>> Adding convenient methods that take a Charset to several classes that
>> have already had methods with a Charset/Encoding name as a parameter.
>> To avoid any impact on compatibility and JCK tests, we've kept the
>> existing methods virtually untouched except making a reference to the
>> new ones to encourage the use of these new methods going forward. The
>> javadocs of the new methods however, may be more complete with
>> details on handling edge cases / Exceptions.
>>
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8183743
>> webrev:
>> http://cr.openjdk.java.net/~joehw/jdk10/8183743/webrev/index.html
> I looked through the javadoc for the updated/new methods and it mostly
> looks good.

Thanks for all of the reviews!
>
> I think URLDecoder.decode methods should have @throws
> IllegalArgumentException. I realize it's implementation specific as to
> whether IAE is thrown with bad input but given that the RI does throw
> IAE then consumers of the API should be prepared for it. The @implNote
> can stay and probably should be copied into the legacy decode method too.

I added @throws IAE. On a 2nd thought, would that give no flexibility to
alternative impls as the general (class) spec had given? With this
addition, it becomes a requirement.
>
> One of the new constructors on Scanner needs @since 10.

Fixed.

-Joe

>
> -Alan
Reply | Threaded
Open this post in threaded view
|

Re: RFR (JDK10) 8183743: Umbrella: add overloads that take a Charset parameter

Alan Bateman


On 01/12/2017 20:35, Joe Wang wrote:

> :
>
>>
>> I think URLDecoder.decode methods should have @throws
>> IllegalArgumentException. I realize it's implementation specific as
>> to whether IAE is thrown with bad input but given that the RI does
>> throw IAE then consumers of the API should be prepared for it. The
>> @implNote can stay and probably should be copied into the legacy
>> decode method too.
>
> I added @throws IAE. On a 2nd thought, would that give no flexibility
> to alternative impls as the general (class) spec had given? With this
> addition, it becomes a requirement.
If the @throws description starts with "if the implementation ..." then
it should be clear that it is optional.

-Alan
Reply | Threaded
Open this post in threaded view
|

Re: RFR (JDK10) 8183743: Umbrella: add overloads that take a Charset parameter

Joe Wang


On 12/3/17, 11:34 AM, Alan Bateman wrote:

>
>
> On 01/12/2017 20:35, Joe Wang wrote:
>> :
>>
>>>
>>> I think URLDecoder.decode methods should have @throws
>>> IllegalArgumentException. I realize it's implementation specific as
>>> to whether IAE is thrown with bad input but given that the RI does
>>> throw IAE then consumers of the API should be prepared for it. The
>>> @implNote can stay and probably should be copied into the legacy
>>> decode method too.
>>
>> I added @throws IAE. On a 2nd thought, would that give no flexibility
>> to alternative impls as the general (class) spec had given? With this
>> addition, it becomes a requirement.
> If the @throws description starts with "if the implementation ..."
> then it should be clear that it is optional.

Thanks Alan. I made the change accordingly.
http://cr.openjdk.java.net/~joehw/jdk10/8183743/webrev/index.html

-Joe

>
> -Alan
Reply | Threaded
Open this post in threaded view
|

Re: RFR (JDK10) 8183743: Umbrella: add overloads that take a Charset parameter

Joe Wang
Thanks Roger, Alan for the comments!

I've updated the webrev accordingly. Here's the current webrev:
http://cr.openjdk.java.net/~joehw/jdk10/8183743/webrev/index.html

-Joe

On 12/4/17, 7:52 PM, Joe Wang wrote:

>
>
> On 12/3/17, 11:34 AM, Alan Bateman wrote:
>>
>>
>> On 01/12/2017 20:35, Joe Wang wrote:
>>> :
>>>
>>>>
>>>> I think URLDecoder.decode methods should have @throws
>>>> IllegalArgumentException. I realize it's implementation specific as
>>>> to whether IAE is thrown with bad input but given that the RI does
>>>> throw IAE then consumers of the API should be prepared for it. The
>>>> @implNote can stay and probably should be copied into the legacy
>>>> decode method too.
>>>
>>> I added @throws IAE. On a 2nd thought, would that give no
>>> flexibility to alternative impls as the general (class) spec had
>>> given? With this addition, it becomes a requirement.
>> If the @throws description starts with "if the implementation ..."
>> then it should be clear that it is optional.
>
> Thanks Alan. I made the change accordingly.
> http://cr.openjdk.java.net/~joehw/jdk10/8183743/webrev/index.html
>
> -Joe
>
>>
>> -Alan
Reply | Threaded
Open this post in threaded view
|

Re: RFR (JDK10) 8183743: Umbrella: add overloads that take a Charset parameter

roger riggs
Hi Joe,

The new tests using testng look good, a few comments:

  - BAOS/EncodingTest.java
    70:  The message gets printed if the condition is not true; so
"results do not might" might read better
     (also, there is Assert.assertEquals(str1, str2, msg) - that
compares objects with .equals())
    A message in nice, though in many cases the testng generated
exception/message is sufficient
    especially for equals.

  - PrintStream/EncodingTest.java
    92: out should not be null; its probably a test bug;  the test
should be removed

- PrintWriter/EncodingTest.java
     93: out should not be null; its probably a test bug;  the test
should be removed

Thanks, Roger


On 12/8/2017 7:29 PM, Joe Wang wrote:

> Thanks Roger, Alan for the comments!
>
> I've updated the webrev accordingly. Here's the current webrev:
> http://cr.openjdk.java.net/~joehw/jdk10/8183743/webrev/index.html
>
> -Joe
>
> On 12/4/17, 7:52 PM, Joe Wang wrote:
>>
>>
>> On 12/3/17, 11:34 AM, Alan Bateman wrote:
>>>
>>>
>>> On 01/12/2017 20:35, Joe Wang wrote:
>>>> :
>>>>
>>>>>
>>>>> I think URLDecoder.decode methods should have @throws
>>>>> IllegalArgumentException. I realize it's implementation specific
>>>>> as to whether IAE is thrown with bad input but given that the RI
>>>>> does throw IAE then consumers of the API should be prepared for
>>>>> it. The @implNote can stay and probably should be copied into the
>>>>> legacy decode method too.
>>>>
>>>> I added @throws IAE. On a 2nd thought, would that give no
>>>> flexibility to alternative impls as the general (class) spec had
>>>> given? With this addition, it becomes a requirement.
>>> If the @throws description starts with "if the implementation ..."
>>> then it should be clear that it is optional.
>>
>> Thanks Alan. I made the change accordingly.
>> http://cr.openjdk.java.net/~joehw/jdk10/8183743/webrev/index.html
>>
>> -Joe
>>
>>>
>>> -Alan

Reply | Threaded
Open this post in threaded view
|

Re: RFR (JDK10) 8183743: Umbrella: add overloads that take a Charset parameter

Joe Wang

On 12/11/17, 11:18 AM, Roger Riggs wrote:

> Hi Joe,
>
> The new tests using testng look good, a few comments:
>
>  - BAOS/EncodingTest.java
>    70:  The message gets printed if the condition is not true; so
> "results do not might" might read better
>     (also, there is Assert.assertEquals(str1, str2, msg) - that
> compares objects with .equals())
>    A message in nice, though in many cases the testng generated
> exception/message is sufficient
>    especially for equals.

Assert.assertEquals it is! Removed other debug printout.

>
>  - PrintStream/EncodingTest.java
>    92: out should not be null; its probably a test bug;  the test
> should be removed

Done.
>
> - PrintWriter/EncodingTest.java
>     93: out should not be null; its probably a test bug;  the test
> should be removed

Done.

Updated webrev:
http://cr.openjdk.java.net/~joehw/jdk10/8183743/webrev/index.html

Thanks,
Joe

>
> Thanks, Roger
>
>
> On 12/8/2017 7:29 PM, Joe Wang wrote:
>> Thanks Roger, Alan for the comments!
>>
>> I've updated the webrev accordingly. Here's the current webrev:
>> http://cr.openjdk.java.net/~joehw/jdk10/8183743/webrev/index.html
>>
>> -Joe
>>
>> On 12/4/17, 7:52 PM, Joe Wang wrote:
>>>
>>>
>>> On 12/3/17, 11:34 AM, Alan Bateman wrote:
>>>>
>>>>
>>>> On 01/12/2017 20:35, Joe Wang wrote:
>>>>> :
>>>>>
>>>>>>
>>>>>> I think URLDecoder.decode methods should have @throws
>>>>>> IllegalArgumentException. I realize it's implementation specific
>>>>>> as to whether IAE is thrown with bad input but given that the RI
>>>>>> does throw IAE then consumers of the API should be prepared for
>>>>>> it. The @implNote can stay and probably should be copied into the
>>>>>> legacy decode method too.
>>>>>
>>>>> I added @throws IAE. On a 2nd thought, would that give no
>>>>> flexibility to alternative impls as the general (class) spec had
>>>>> given? With this addition, it becomes a requirement.
>>>> If the @throws description starts with "if the implementation ..."
>>>> then it should be clear that it is optional.
>>>
>>> Thanks Alan. I made the change accordingly.
>>> http://cr.openjdk.java.net/~joehw/jdk10/8183743/webrev/index.html
>>>
>>> -Joe
>>>
>>>>
>>>> -Alan
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR (JDK10) 8183743: Umbrella: add overloads that take a Charset parameter

roger riggs
Hi Joe,

Looks good;  +1

Regards, Roger

On 12/11/2017 3:04 PM, Joe Wang wrote:

>
> On 12/11/17, 11:18 AM, Roger Riggs wrote:
>> Hi Joe,
>>
>> The new tests using testng look good, a few comments:
>>
>>  - BAOS/EncodingTest.java
>>    70:  The message gets printed if the condition is not true; so
>> "results do not might" might read better
>>     (also, there is Assert.assertEquals(str1, str2, msg) - that
>> compares objects with .equals())
>>    A message in nice, though in many cases the testng generated
>> exception/message is sufficient
>>    especially for equals.
>
> Assert.assertEquals it is! Removed other debug printout.
>
>>
>>  - PrintStream/EncodingTest.java
>>    92: out should not be null; its probably a test bug;  the test
>> should be removed
>
> Done.
>>
>> - PrintWriter/EncodingTest.java
>>     93: out should not be null; its probably a test bug;  the test
>> should be removed
>
> Done.
>
> Updated webrev:
> http://cr.openjdk.java.net/~joehw/jdk10/8183743/webrev/index.html
>
> Thanks,
> Joe
>
>>
>> Thanks, Roger
>>
>>
>> On 12/8/2017 7:29 PM, Joe Wang wrote:
>>> Thanks Roger, Alan for the comments!
>>>
>>> I've updated the webrev accordingly. Here's the current webrev:
>>> http://cr.openjdk.java.net/~joehw/jdk10/8183743/webrev/index.html
>>>
>>> -Joe
>>>
>>> On 12/4/17, 7:52 PM, Joe Wang wrote:
>>>>
>>>>
>>>> On 12/3/17, 11:34 AM, Alan Bateman wrote:
>>>>>
>>>>>
>>>>> On 01/12/2017 20:35, Joe Wang wrote:
>>>>>> :
>>>>>>
>>>>>>>
>>>>>>> I think URLDecoder.decode methods should have @throws
>>>>>>> IllegalArgumentException. I realize it's implementation specific
>>>>>>> as to whether IAE is thrown with bad input but given that the RI
>>>>>>> does throw IAE then consumers of the API should be prepared for
>>>>>>> it. The @implNote can stay and probably should be copied into
>>>>>>> the legacy decode method too.
>>>>>>
>>>>>> I added @throws IAE. On a 2nd thought, would that give no
>>>>>> flexibility to alternative impls as the general (class) spec had
>>>>>> given? With this addition, it becomes a requirement.
>>>>> If the @throws description starts with "if the implementation ..."
>>>>> then it should be clear that it is optional.
>>>>
>>>> Thanks Alan. I made the change accordingly.
>>>> http://cr.openjdk.java.net/~joehw/jdk10/8183743/webrev/index.html
>>>>
>>>> -Joe
>>>>
>>>>>
>>>>> -Alan
>>

Reply | Threaded
Open this post in threaded view
|

Re: RFR (JDK10) 8183743: Umbrella: add overloads that take a Charset parameter

Joe Wang
Thanks Roger, Alan. I've pushed the changeset after re-ran the core tests.

-Joe

On 12/12/17, 8:47 AM, Roger Riggs wrote:

> Hi Joe,
>
> Looks good;  +1
>
> Regards, Roger
>
> On 12/11/2017 3:04 PM, Joe Wang wrote:
>>
>> On 12/11/17, 11:18 AM, Roger Riggs wrote:
>>> Hi Joe,
>>>
>>> The new tests using testng look good, a few comments:
>>>
>>>  - BAOS/EncodingTest.java
>>>    70:  The message gets printed if the condition is not true; so
>>> "results do not might" might read better
>>>     (also, there is Assert.assertEquals(str1, str2, msg) - that
>>> compares objects with .equals())
>>>    A message in nice, though in many cases the testng generated
>>> exception/message is sufficient
>>>    especially for equals.
>>
>> Assert.assertEquals it is! Removed other debug printout.
>>
>>>
>>>  - PrintStream/EncodingTest.java
>>>    92: out should not be null; its probably a test bug;  the test
>>> should be removed
>>
>> Done.
>>>
>>> - PrintWriter/EncodingTest.java
>>>     93: out should not be null; its probably a test bug;  the test
>>> should be removed
>>
>> Done.
>>
>> Updated webrev:
>> http://cr.openjdk.java.net/~joehw/jdk10/8183743/webrev/index.html
>>
>> Thanks,
>> Joe
>>
>>>
>>> Thanks, Roger
>>>
>>>
>>> On 12/8/2017 7:29 PM, Joe Wang wrote:
>>>> Thanks Roger, Alan for the comments!
>>>>
>>>> I've updated the webrev accordingly. Here's the current webrev:
>>>> http://cr.openjdk.java.net/~joehw/jdk10/8183743/webrev/index.html
>>>>
>>>> -Joe
>>>>
>>>> On 12/4/17, 7:52 PM, Joe Wang wrote:
>>>>>
>>>>>
>>>>> On 12/3/17, 11:34 AM, Alan Bateman wrote:
>>>>>>
>>>>>>
>>>>>> On 01/12/2017 20:35, Joe Wang wrote:
>>>>>>> :
>>>>>>>
>>>>>>>>
>>>>>>>> I think URLDecoder.decode methods should have @throws
>>>>>>>> IllegalArgumentException. I realize it's implementation
>>>>>>>> specific as to whether IAE is thrown with bad input but given
>>>>>>>> that the RI does throw IAE then consumers of the API should be
>>>>>>>> prepared for it. The @implNote can stay and probably should be
>>>>>>>> copied into the legacy decode method too.
>>>>>>>
>>>>>>> I added @throws IAE. On a 2nd thought, would that give no
>>>>>>> flexibility to alternative impls as the general (class) spec had
>>>>>>> given? With this addition, it becomes a requirement.
>>>>>> If the @throws description starts with "if the implementation
>>>>>> ..." then it should be clear that it is optional.
>>>>>
>>>>> Thanks Alan. I made the change accordingly.
>>>>> http://cr.openjdk.java.net/~joehw/jdk10/8183743/webrev/index.html
>>>>>
>>>>> -Joe
>>>>>
>>>>>>
>>>>>> -Alan
>>>
>