On Fri, Feb 9, 2018 at 11:05 AM, Jaikiran Pai <jai.forums2...@gmail.com> wrote:
> > On 09/02/18 2:47 PM, Dominique Devienne wrote: > >> On Fri, Feb 9, 2018 at 10:06 AM, Jaikiran Pai <jai.forums2...@gmail.com> >> wrote: >> >> I need some inputs on how we should go about this specific change/test? >>> Should this test continue to expect a exception or is it fine to expect >>> that target to complete cleanly (without copying anything)? >>> >>> What's the source of the NPE? >> If it's an implementation detail, then it's fine to no longer throw IMHO. >> > We have FileNameMapper interface which has a: > > String[] mapFileName(String sourceFileName); > > API. The contract/javadoc of this API says: > > * Returns an array containing the target filename(s) for the > * given source file. > * > * <p>if the given rule doesn't apply to the source file, > * implementation must return null.... > > We have an implementation of this interface, the IdentityMapper, whose > implementation so far, IMO, wasn't following this contract. If it was > passed a null source file name, that implementation would return an > String[] with one element and the contained element in that array would be > null. > > Callers of this API are expected to understand that the API itself might > return null, so they used to just check the return value and not its > contents. They would then go and start working on these individual elements > in the array and start running into NPEs in a bunch of places. > > The change I made was to the implementation of IdentityMapper, so that it > returns null instead of an array with one null element, when the input to > it is null. That way, the callers' logic of checking the return value for > null, is enough for them to skip such resources. > > So to me, this does look like something that we should take care off in > that specific test, so that it no longer expects a build exception to be > thrown. Agreed. +1. Thanks for the details and "walking me through the code". --DD
