Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/462#discussion_r90756006
--- Diff:
core/src/main/java/org/apache/brooklyn/util/core/xstream/ClassRenamingMapper.java
---
@@ -42,7 +42,7 @@ public ClassRenamingMapper(Mapper wrapped, Map<String,
String> nameToType) {
@Override
public Class<?> realClass(String elementName) {
- Optional<String> elementNameOpt =
Reflections.tryFindMappedName(nameToType, elementName);
+ Maybe<String> elementNameOpt =
Reflections.findMappedNameMaybe(nameToType, elementName);
--- End diff --
My question about renaming wasn't so much about why change to return a
`Maybe`. It's why change the method name? I prefer `tryFindMappedName` because
it follows the guava naming convention. We use that naming convention in quite
a few other places in Brooklyn as well.
I see now why - the previous `tryFindMappedName` is deprecated in favour of
`findMappedNameMaybe` so that it can have a different return type. That's
really annoying (that we're creating a different method naming convention so as
to change the return type). But I'm not sure how better to tackle that while
still preserving backwards compatibility!
As for null, you've changed the contract. Maybe it's fine that the super
throws the `NullPointerException` rather than us checking so not a big deal.
But on balance, I'd prefer to report a null present value here, rather than
calling `super.realClass(null)`, which could get into completely different code
before an NPE is thrown. No strong feelings though.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
