[2962] Mismatch between mechanism to check if nullChannel bean exists and mechanism to fetch that bean#2963
Conversation
|
@michaelwiles Please sign the Contributor License Agreement! Click here to manually synchronize the status of this Pull Request. See the FAQ for frequently asked questions. |
michaelwiles
changed the title
|
@michaelwiles Thank you for signing the Contributor License Agreement! |
michaelwiles
force-pushed
the
master
branch
2 times, most recently
from
1061b61 to
3e162a7
Compare
|
Added a test |
with tests
artembilan
left a comment
artembilan
left a comment
There was a problem hiding this comment.
Here is some my review.
Thanks
| } | ||
| } | ||
| } | ||
| // don't think it will hurt to always find the parent |
There was a problem hiding this comment.
I'm not sure that we really are going to accept so internal thoughts as comments.
Why can't we really live without these comments at all?
There was a problem hiding this comment.
Agreed and no problem
There was a problem hiding this comment.
I guess I was just explaining my thinking
| beanFactory = ((HierarchicalBeanFactory) beanFactory).getParentBeanFactory(); | ||
| } | ||
| // will definitely be found as containsBean returned true - but also want to be defensive in case of NPE | ||
| } while (nullChannelDefinition == null && beanFactory != null); // not sure if beanFactroy not null is necessary |
There was a problem hiding this comment.
The while must be on a new line.
I think we indeed don't need to check for beanFactory != null, since you are right that containsBean(IntegrationContextUtils.NULL_CHANNEL_BEAN_NAME) does the trick for us do not fail with the infinite loop.
Although if Checkstyle or Sonar Qube will require it I would just live with an extra check in this condition.
There was a problem hiding this comment.
Yes, I'll ensure that the coding style is adhered to. Finding meeting coding guidelines quite challenging
There was a problem hiding this comment.
comments and null check on bean factory removed.
michaelwiles
force-pushed
the
master
branch
2 times, most recently
from
851b230 to
6879a14
Compare
artembilan
left a comment
artembilan
left a comment
There was a problem hiding this comment.
Please, add your name to @author list for all affected classes.
That's all - otherwise LGTM
|
should I rebase into 1 commit? |
|
No, we do that on merge. |
|
... and cherry-picked to Thank you very much for the contribution; looking forward for more! |
3 participants
…2962
Pull request to resolve #2962
I did it very defensively so with lots of type checking etc as I can't be sure of the parent factories. I think this is all necessary as we're having to fetch the bean definition from the outside.
Also apologies if the formatting is not correct. I did try to use the eclipse formatter spec but the section of code I changed may not be formatted correctly.
Also see some of my inline comments.