On Mon, 24 Apr 2023 19:25:59 GMT, Archie Cobbs <aco...@openjdk.org> wrote:
>> The `Introspector` class was never updated to include `default` methods
>> inherited from interfaces.
>>
>> This patch attempts to fix that omission.
>
> 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.
Marked as reviewed by aivanov (Reviewer).
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.
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.
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 ")));
-------------
PR Review: https://git.openjdk.org/jdk/pull/13544#pullrequestreview-1400494157
PR Review Comment: https://git.openjdk.org/jdk/pull/13544#discussion_r1176886529
PR Review Comment: https://git.openjdk.org/jdk/pull/13544#discussion_r1176890225
PR Review Comment: https://git.openjdk.org/jdk/pull/13544#discussion_r1176888835
