On Fri, Feb 9, 2018 at 11:05 AM, Jaikiran Pai <jai.forums2...@gmail.com>
> 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>
>> 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
> 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
Agreed. +1. Thanks for the details and "walking me through the code". --DD