Closed tisonkun closed 4 years ago
+1
Thanks for your review @yiheng . Formally we send an approved review feedback for an approval.
Waiting for another reviewer @guangxuCheng since this pull request touch a lot of files.
@TisonKun I think we should add a checker for the licence to ensure that the header of the file contains the licence.
@guangxuCheng good point! Implemented by apache-rat-plugin https://github.com/Tencent/TubeMQ/pull/55/commits/2ea61bf768b1df80cc59cd2f73194d65b0ae19c0
LGTM +1
Thanks for your reviews @yiheng @guangxuCheng . I narrow the pending issues to three below.
ConstantNameCheck
.Generally disabled on
private static final
so that we narrow the impact tologger
. AllLogger
are nowlogger
and we can always do a reformat if further concern occurs. The problem is that I modify severallogger
fromstatic final
toprivate static final
but I think it is OK because all of them is not and should not be used outside the class.MethodNameCheck
I have no idea whether changes to method name would impact existing dependencies to TubeMQ. If we all agree that we can rename
Subscribe
assubscribe
andTestXXX
/test_xxx
astestXxx
I will turn on the checker and do a general reformatting.RedundantModifier
Mostly it is about
final
in methods of interfaces. An ideal way in my mind is reformatted our method signature as the format belowBut it possibly impacts too much. An alternative is that we exclude
METHOD_DEF
property. It also makes sense to me.CC @JunpingDu @gosonzhang