gaogaotiantian commented on code in PR #57770:
URL: https://github.com/apache/spark/pull/57770#discussion_r3754420212
##########
dev/run-tests.py:
##########
@@ -69,43 +69,42 @@ def determine_java_executable():
#
-------------------------------------------------------------------------------------------------
-def set_title_and_block(title, err_block):
- os.environ["CURRENT_BLOCK"] = str(ERROR_CODES[err_block])
- line_str = "=" * 72
-
- print("")
- print(line_str)
- print(title)
- print(line_str)
+_in_titled_block = False
@contextmanager
-def group_in_github_actions(title):
+def titled_block(title):
+ global _in_titled_block
Review Comment:
The point it to avoid polluting global name space. If a variable is only
used by a single function, it should be part of that function. I'm okay if you
want to make this function a class so you can properly define `_entered` as a
class attribute, but I don't like having global variables for a single
function. It's not obvious that `_in_titled_block` is designed for
`titled_block` only. It's possible that in the future someone accidentally
insert some code between `_in_titled_block` and `titled_block` which will make
it more difficult to catch the fact. It's also possible that someone uses
`_in_titled_block` in an unexpected way because it's a global variable and
conflict the existing usage.
I don't think this is a huge deal, but I think it's a safer way to implement
this guard. I know global variables are a more common way to do things like
this, but most users write their code for convenience, not stability.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
