Defer Scannable#name() fallback logic
#3900
Conversation
| @@ -449,7 +449,7 @@ default String name() { | |||
| .map(s -> s.scan(Attr.NAME)) | |||
| .filter(Objects::nonNull) | |||
| .findFirst() | |||
| .orElse(stepName()); | |||
| .orElseGet(this::stepName); | |||
There was a problem hiding this comment.
Two things:
- In some cases
#stepNameis very expensive. I'll file a separate PR with benchmark for that, and link it here afterwards. - I checked other
Optional#orElsecalls inreactor-core; the remainder accept a constant, which is fine. That said, if Reactor would like to automatically catch cases such as this one, @rickie or myself would be happy to contribute a (limited, if desired) integration with Error Prone Support, enabling checks such as this one.
There was a problem hiding this comment.
I'll file a separate PR with benchmark for that, and link it here afterwards.
Done: #3901.
There was a problem hiding this comment.
Sounds reasonable to move the changes from this PR to #3901 as they are related and the whole context is there.
Stephan202
force-pushed
the
sschroevers/defer-step-name-construction
branch
from
e64527a to
009ec89
Compare
There was a problem hiding this comment.
Assuming Scannable#stepName does not create important side effects, this should be a net-positive. 💪
Thanks for filing the PR @Stephan202!
There was a problem hiding this comment.
@Stephan202 thanks for the PR. It looks promising. As suggested in the comment, can you please merge this work with the other PR? Thanks.
| @@ -449,7 +449,7 @@ default String name() { | |||
| .map(s -> s.scan(Attr.NAME)) | |||
| .filter(Objects::nonNull) | |||
| .findFirst() | |||
| .orElse(stepName()); | |||
| .orElseGet(this::stepName); | |||
There was a problem hiding this comment.
Sounds reasonable to move the changes from this PR to #3901 as they are related and the whole context is there.
Depending on the implementation,
Scannable#stepName()may be expensiveto invoke. As such this should only happen when necessary.