#164 - Annotate API methods in the Configurator extension point #165
Conversation
…igurator issues
…de method to throw the new exception type
|
@oleg-nenashev no 👎 from me on this specific topic, so I'm fine with merging |
|
I'm in the middle of the relocation process, so would be able to pay attention only week or so after, so please don't wait for me if it matters |
|
|
||
| private final static Logger logger = Logger.getLogger(Configurator.class.getName()); | ||
|
|
||
|
|
||
| @CheckForNull | ||
| public static RootElementConfigurator lookupRootElement(String name) { |
There was a problem hiding this comment.
Probably should be Optional<...> as a return type, so we could use orElse to throw exceptions instead of verbosive if configurator == null
There was a problem hiding this comment.
Maybe, let's wait for what the maintainers say
| * @throws ConfiguratorException Configurator is not found | ||
| */ | ||
| @Nonnull | ||
| public static Configurator lookupOrFail(Type type) throws ConfiguratorException { |
There was a problem hiding this comment.
no need in such method if we get Optional one
| @CheckForNull | ||
| private final ElementConfigurator configurator; | ||
|
|
||
| public ConfiguratorException(@CheckForNull ElementConfigurator configurator, @CheckForNull String message, @CheckForNull Throwable cause) { |
There was a problem hiding this comment.
I'd like to see builder instead of all combinations of fields with two default constructors (String, Thr), (String).
I think you should always write a comment if you wrap another exception
|
@ewelinawilkosz @MadsNielsen any comments here? |
|
no comments from my site, great work! |
|
@oleg-nenashev Looks fine to me @lanwen Create a pull request once this has been merged with the proposed changes. I like most of the suggestion. Except i think the builder pattern might not be strictly necessary, when you only have one 3 arg constructor, that is manageble to work with in a proper editor. But 👍 from here. |
|
Interesting... :) Who does the review should fix the found notes? |
|
@lanwen I can do it :) ~ I'd just keep the optimization seperate, easier to review a smaller change like that. |
|
@lanwen @MadsNielsen I am happy to switch APIs to optional if it helps, but I would also do it in a follow-up request. If anyone wants to do it, it should be a refactoring widely applied to all API. Otherwise |
|
@oleg-nenashev Then i'm ok with the merge. @ewelinawilkosz you ok as well? @lanwen I'd be happy for you to review a pull request were we switch to this modern API, you seem to very familiar with it so it would make sense for you to review such a change seperately. |
|
yes! ok as well, just wanted to get a green light again from Oleg, since I got lost in the discussion :) |
See #164. Just a first refactorings drop, which should improve issue diagnosability a bit
ElementConverterinterface which is a top-level interface for all convertersConverterException, switch convert() APIs to it@reviewbybees