Apache9 commented on a change in pull request #2714:
URL: https://github.com/apache/hbase/pull/2714#discussion_r536482171
##########
File path: dev-support/hbase-personality.sh
##########
@@ -816,6 +816,69 @@ function hbaseanti_patchfile
return 0
}
+######################################
+
+add_test_type visible-for-testing
+
+## @description visible-for-testing file filter
+## @audience private
+## @stability evolving
+## @param filename
+function visible-for-testing_filefilter
+{
+ local filename=$1
+
+ if [[ ${filename} =~ \.java$ ]]; then
+ add_test visible-for-testing
+ fi
+}
+
+## @description visible-for-testing patch file check
+## @audience private
+## @stability evolving
+## @param repostatus
+function visible-for-testing_precompile
+{
+ local repostatus=$1
+ local logfile="${PATCH_DIR}/patch-visible-for-testing.txt"
+ local errors
+
+ if [[ "${repostatus}" = branch ]]; then
+ return 0
+ fi
+
+ if ! verify_needed_test visible-for-testing; then
+ return 0
+ fi
+
+ big_console_header "VisibleForTesting plugin: ${BUILDMODE}"
+ if [[ "${BUILDMODE}" == "full" ]]; then
+ yetus_debug "going to check the whole repo"
+ find -type f -name *.java | grep -v generated | xargs grep -l
"@InterfaceAudience.Public\|@InterfaceAudience.LimitedPrivate" | xargs grep -l
@VisibleForTesting >> ${logfile}
Review comment:
If we are in a company I think I will agree with you to set a strict
rule to prevent any potential risks. But in a open source community, I think
the starting point is a bit different. I agree with you that, a
VisibleForTesting annotation is like a javadoc, but I do not think this is
strong enough reason to disable one way, we could accept both ways, to suit
more developers in the world. As you point out Guava has a history to introduce
conflicts, but our solution is to shade it and purge it from the public API so
it will not effect downstream users, but we still want to use it instead of
disable it right? It is widely used across the java developers.
FWIW, we have not seen a plan of google to remove this class. It is also
used in Android
https://developer.android.com/reference/androidx/annotation/VisibleForTesting
And it is not very difficult to just purge it from the source code in the
future if we have more strong reasons to disable it, just like what you have
done. You can use semantice search in IDE instead of text search so you could
find all the reference quickly and accurately.
So I prefer that, we have tool to disable the usage of this annotation in
IA.Public and IA.LimitedPrivate classes, as this is the actual anti-pattern
here. And in the future, if we find out that VisibleForTesting also introduces
problems even in IA.Private classes, we could just ban it in maven enforcer.
What do you think?
Thanks.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
