brooklyn-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From aledsage <>
Subject [GitHub] brooklyn-server pull request #462: Inherit config default values
Date Sat, 03 Dec 2016 11:57:13 GMT
Github user aledsage commented on a diff in the pull request:
    --- Diff: core/src/main/java/org/apache/brooklyn/util/core/xstream/
    @@ -42,7 +42,7 @@ public ClassRenamingMapper(Mapper wrapped, Map<String, String>
nameToType) {
         public Class<?> realClass(String elementName) {
    -        Optional<String> elementNameOpt = Reflections.tryFindMappedName(nameToType,
    +        Maybe<String> elementNameOpt = Reflections.findMappedNameMaybe(nameToType,
    --- 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 or file a JIRA ticket
with INFRA.

View raw message