> On 4 Nov 2015, at 04:50, Mandy Chung <mandy.ch...@oracle.com> wrote:
>
>
>> On Nov 3, 2015, at 3:28 AM, Paul Sandoz <paul.san...@oracle.com> wrote:
>>
>> Hi Mandy,
>>
>> This is very nice work.
>>
>> Comments below, mostly minor stuff.
>>
>
> Thanks for the review.
>
> I fixed most of the comments below. One question:
>
> Is the name “StackStream" inappropriate and its subtypes? I prefer
> StackStream to StackTraverser.
>
I think using Stream in the name gives the wrong impression. It’s not a subtype
of j.u.Stream, it’s the thing that is used as a source for a Stream, in some
cases.
IIUC it’s really two things, a form of Spliterator that supports sequential
traversal of stack elements via tryAdvanced/forEachRemaining, and a thing to
directly query stack elements.
>
>>
>> StackStream
>> —
>>
>> Suggest using something other than Stream in the name is this is really
>> about Spliterator and traversal e.g. perhaps StackTraverser. Same for
>> sub-types of StackStream.
>>
>> 494 @Override
>> 495 public void forEachRemaining(Consumer<? super StackFrame> action) {
>> 496 throw new UnsupportedOperationException("Specialized
>> StackWalker");
>> 497 }
>> 498
>> 499 @Override
>> 500 public boolean tryAdvance(Consumer<? super StackFrame> action) {
>> 501 throw new UnsupportedOperationException("Specialized
>> StackWalker");
>> 502 }
>>
>> Is this required?
>>
>
> CallerClassStream and StackTrace don’t support to use with Stream API for
> performance.
Then perhaps those classes should implement those methods to throw USO? or
there should be a sub-type which indicates Spliterator traversal is not
supported? Then i think it becomes much clearer on what sub-types support what
contract.
Or maybe the Spliterator can be pushed down to sub-types that are used as the
source for a Stream? (I have not re-looked into the details to how awkward this
might become.)
>
>>
>> 238 private final List<? extends WalkerOption> options;
>>
>> Do you require the bound here?
>>
>
> Sorry WalkerOption has been removed. I only updated the API but not
> webrev.00.
>
Ok, thats good!
>>
>> 241 /**
>> 242 * Gets a {@code StackWalker} instance.
>> 243 *
>> 244 * <p> This {@code StackWalker} obtains the method name and the
>> class name
>> 245 * with all {@linkplain StackWalker.Option#SHOW_HIDDEN_FRAMES hidden
>> frames}
>> 246 * skipped.
>> 247 */
>> 248 public StackWalker() {
>>
>> s/Gets/Creates
>>
>> Same for other constructor methods.
>>
>>
>> 252 /**
>> 253 * Gets a {@code StackWalker} instance with the given options
>> specifying
>> 254 * the stack frame information it can access. If no option is
>> specified, this
>> 255 * {@code StackWalker} obtains the method name and the class name
>> 256 * with all {@linkplain StackWalker.Option#SHOW_HIDDEN_FRAMES hidden
>> frames}
>> 257 * skipped.
>>
>> If no options are given…
>>
>> It might be easier to state as if the options were {CLASS_REFERENCE,
>> SHOW_REFLECT_FRAMES}, perhaps it depends if the default is someone under
>> specified i.e. is it guaranteed that the option set will be the complement
>> of the single set SHOW_HIDDEN_FRAMES?
>
> In fact the default can’t be expressed with the current options. This is a
> good point though and I have been considering adding SHOW_METHOD_INFO which
> will be the default. I’ll update the API.
>
Ok.
>>
>> I guess any unrecognized options are ignored?
>>
>
> Now it takes only Option enums.
>
That’s better.
>
>>
>> 430 /**
>> 431 * Gets the {@code Class} object of the caller invoking the method
>> 432 * that calls this {@code getCallerClass} method.
>> 433 *
>> 434 * <p>This is equivalent to calling
>> 435 * <blockquote>
>> 436 * <pre>{@code walk((s) ->}
>>
>> } -> {
>>
>> 437 * s.map(StackFrame::getDeclaringClass)
>> 438 * .skip(2)
>> 439 * .findFirst()).orElse(Thread.currentThread().getClass());
>> 440 * </pre></blockquote>
>>
>>
>> 441 *
>> 442 * <p>For example, {@code Util::getResourceBundle} loads a resource
>> bundle on behalf
>> 443 * of the caller. It calls this {@code getCallerClass} method to
>> find the method
>> 444 * calling {@code Util::getResourceBundle} and use the caller's
>> class loader to
>> 445 * load the resource bundle. The caller class in this example is the
>> {@code Foo} class.
>>
>>
>> use @apiNote
>>
>>
>> BatchSizeMapper
>> —
>>
>> As usual i always advice using testng and data providers, up to you.
>
> I started this one with TestNG but this data provider requires the method to
> return Object[][]. I automatically resorted to old-school test :)
>
:-) it’s more of an annoyance, i often compose using a list then convert. I can
help out if you like.
IIRC it may also possible to return Iterable<Object[]> or Iterator<Object[]> i
cannot remember, but don’t expect the TestNG documentation to tell you that!,
you need to look into the source code to work that out.
Paul.
