Re: 8159248: ModuleFinder.of spec + 8158456: ModuleDescriptor.read does not verify dependence on java.base

[Alan Bateman]

On 16/06/2016 19:11, Mandy Chung wrote:

On Jun 16, 2016, at 9:44 AM, Alan Bateman  wrote:

On 16/06/2016 17:31, Mandy Chung wrote:


:
ModuleFinder.of spec describes the possible cases where FindException may be 
thrown by a module finder’s find or findAll methods.

It would be useful to have the @throws FindException if an error occurs finding 
all modules in find and findAll methods to link to such description.

My concern with this is that it will clutter the @throws description if we 
attempt to link to all cases (there are too many).

One idea is to add section header in ModuleFinder.of error cases and then
@throws FindException if an error occurs finding modules, as described in ModuleFinder::of


I'll think about it but ModuleFinder.of is just one implementation.

-Alan

Re: 8159248: ModuleFinder.of spec + 8158456: ModuleDescriptor.read does not verify dependence on java.base

[Alan Bateman]

On 16/06/2016 17:47, Paul Benedict wrote:

Please definitely list all such possible exceptions. The knowledge 
outweighs concern for clutter, IMO. I'd rather know the full number of 
ways an API can fail rather than receive a failure that wasn't documented.
There is one exception and all the cases that we can describe are 
already in the javadoc. There are several other cases where we don't 
fail but should, to address those requires several adjustments to the 
builder API and I'd prefer to keep that separate.


-Alan

Re: 8159248: ModuleFinder.of spec + 8158456: ModuleDescriptor.read does not verify dependence on java.base

[Paul Benedict]

Please definitely list all such possible exceptions. The knowledge
outweighs concern for clutter, IMO. I'd rather know the full number of ways
an API can fail rather than receive a failure that wasn't documented.

Cheers,
Paul

On Thu, Jun 16, 2016 at 11:44 AM, Alan Bateman 
wrote:

> On 16/06/2016 17:31, Mandy Chung wrote:
>
> :
>> ModuleFinder.of spec describes the possible cases where FindException may
>> be thrown by a module finder’s find or findAll methods.
>>
>> It would be useful to have the @throws FindException if an error occurs
>> finding all modules in find and findAll methods to link to such description.
>>
> My concern with this is that it will clutter the @throws description if we
> attempt to link to all cases (there are too many).
>
>
>> As I commented offline, it’d be good to link to the definition of legal
>> package name but looks like we need to add that in some appropriate spec
>> before we can do that.
>>
>> There is big overhaul of the builder API needed and I think that is the
> right place to do this. The javadoc update here deliberately delegates to
> avoid repeating everything.
>
> -Alan
>

Re: 8159248: ModuleFinder.of spec + 8158456: ModuleDescriptor.read does not verify dependence on java.base

[Alan Bateman]

On 16/06/2016 17:31, Mandy Chung wrote:


:
ModuleFinder.of spec describes the possible cases where FindException may be 
thrown by a module finder’s find or findAll methods.

It would be useful to have the @throws FindException if an error occurs finding 
all modules in find and findAll methods to link to such description.
My concern with this is that it will clutter the @throws description if 
we attempt to link to all cases (there are too many).




As I commented offline, it’d be good to link to the definition of legal package 
name but looks like we need to add that in some appropriate spec before we can 
do that.

There is big overhaul of the builder API needed and I think that is the 
right place to do this. The javadoc update here deliberately delegates 
to avoid repeating everything.


-Alan

Re: 8159248: ModuleFinder.of spec + 8158456: ModuleDescriptor.read does not verify dependence on java.base

[Mandy Chung]

> On Jun 16, 2016, at 8:33 AM, Alan Bateman  wrote:
> 
> I need a Reviewer for a patch is to address two conformance issues.
> 
> One is that the ModuleFinder.of(Path...) javadoc doesn't make it clear that 
> an exception is thrown when a module descriptor cannot be created for an 
> automatic module. As part of this I've changed the implementation to not 
> silently skip services configuration files or contents that don't parse as 
> identifiers. Also a small change to ModuleDescriptor.Builder in preparation 
> for other changes.
> 
> The other issue is that the requires table in the Module attribute for 
> java.base should be empty and should have at least an entry for java.base for 
> other modules.
> 
> The changes are simple, most of the changes are extending the test coverage:
> 
> http://cr.openjdk.java.net/~alanb/8159248%2b8158456/webrev/

This looks fine.

ModuleFinder.of spec describes the possible cases where FindException may be 
thrown by a module finder’s find or findAll methods.

It would be useful to have the @throws FindException if an error occurs finding 
all modules in find and findAll methods to link to such description.

As I commented offline, it’d be good to link to the definition of legal package 
name but looks like we need to add that in some appropriate spec before we can 
do that.

Mandy

Re: 8159248: ModuleFinder.of spec + 8158456: ModuleDescriptor.read does not verify dependence on java.base

[Chris Hegarty]

> On 16 Jun 2016, at 16:33, Alan Bateman  wrote:
> 
> I need a Reviewer for a patch is to address two conformance issues.
> 
> One is that the ModuleFinder.of(Path...) javadoc doesn't make it clear that 
> an exception is thrown when a module descriptor cannot be created for an 
> automatic module. As part of this I've changed the implementation to not 
> silently skip services configuration files or contents that don't parse as 
> identifiers. Also a small change to ModuleDescriptor.Builder in preparation 
> for other changes.
> 
> The other issue is that the requires table in the Module attribute for 
> java.base should be empty and should have at least an entry for java.base for 
> other modules.
> 
> The changes are simple, most of the changes are extending the test coverage:
> 
> http://cr.openjdk.java.net/~alanb/8159248%2b8158456/webrev/

Both these changes look good to me Alan.

-Chris.