On Tue, 25 Apr 2023 18:31:01 GMT, Alexey Ivanov <aiva...@openjdk.org> wrote:
>> Archie Cobbs has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - Jam lines into 80 columns.
>> - Add more scenarios to the regression test.
>
> test/jdk/java/beans/Introspector/DefaultMethodBeanPropertyTest.java line 184:
>
>> 182: .ifPresent(name -> {
>> 183: throw new Error("property \"" + name + "\" not found in " +
>> type);
>> 184: });
>
> Suggestion:
>
> expected.stream()
> .map(PropertyDescriptor::getName)
> .filter(name -> BeanUtils.getPropertyDescriptor(type, name)
> == null)
> .findFirst()
> .ifPresent(name -> {
> throw new Error("property "" + name + "" not found in " +
> type);
> });
>
> I don't know if there's a common style that's followed in OpenJDK, however,
> aligning wrapped method calls in streams on the dot is the most common way. I
> prefer this style.
>
> Indenting wrapped lines by two spaces doesn't look standard to me, the most
> common indentation for wrapped lines seems to be 8 spaces.
Thanks - fixed in b92726a923b.
> test/jdk/java/beans/Introspector/DefaultMethodBeanPropertyTest.java line 188:
>
>> 186: // Gather actual properties
>> 187: final Set<PropertyDescriptor> actual = Set.of(
>> 188: BeanUtils.getPropertyDescriptors(type));
>
> Suggestion:
>
> final Set<PropertyDescriptor> actual =
> Set.of(BeanUtils.getPropertyDescriptors(type));
>
> May be easier to read? I mean wrapping the entire expression instead of
> breaking the expression.
Thanks - fixed in b92726a923b.
> test/jdk/java/beans/Introspector/DefaultMethodBeanPropertyTest.java line 200:
>
>> 198: + expected.stream()
>> 199: .map(Object::toString)
>> 200: .collect(Collectors.joining("\n ")));
>
> Suggestion:
>
> + actual.stream()
> .map(Object::toString)
> .collect(Collectors.joining("\n "))
> + "\nEXPECTED:\n "
> + expected.stream()
> .map(Object::toString)
> .collect(Collectors.joining("\n ")));
Thanks - fixed in b92726a923b.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/13544#discussion_r1176901780
PR Review Comment: https://git.openjdk.org/jdk/pull/13544#discussion_r1176901938
PR Review Comment: https://git.openjdk.org/jdk/pull/13544#discussion_r1176901865
