[GitHub] [hbase] Apache9 commented on a change in pull request #2714: HBASE-25333 Add a yetus check to disable usage of VisibleForTesting a…

[GitBox <gi...@apache.org>]

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:
users@infra.apache.org