Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Drop superfluous 'global' statements from command.py #621

Merged
merged 2 commits into from
May 25, 2024
Merged

Drop superfluous 'global' statements from command.py #621

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c5baf7a
Drop superfluous 'global' statements from command.py
cottsay Feb 22, 2024
c4ee7d4
Merge branch 'master' into cottsay/superfluous-global-statements
cottsay May 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions colcon_core/command.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,6 @@ def register_command_exit_handler(handler):

:param handler: The callable
"""
global _command_exit_handlers
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think is trivial removing global in relation to _command_exit_handlers that variable is defined as a global variable in line

colcon-core/colcon_core/command.py

Line 78 in 03d6193

_command_exit_handlers = []
.
The prefix global in python is use to be able to read and write a global variable in the context of a specific function. Without that prefix whatever change you do to that variable within the function won't take effect on the global defined variable but a copy of it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without that prefix whatever change you do to that variable within the function won't take effect on the global defined variable but a copy of it.

I don't think this statement is true as written. The global statement makes value replacement operations affect the global variable instead of creating a local variable, but MODIFYING an object behind a global variable is not affected by the global statement at all, and has always been the implicit behavior.

>>> foo = []
>>> def doit():
...   foo.append('hello, world')
... 
>>> doit()
>>> foo
['hello, world']
>>>

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With a replacement-type operation, the global statement is needed:

>>> foo = 0
>>> def doit():
...   foo = 1
... 
>>> doit()
>>> foo
0
>>> def doit_better():
...   global foo
...   foo = 1
... 
>>> doit_better()
>>> foo
1
>>>

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems I have been coding immutable code for long enough that I have forgotten that a lot of data structures are mutable. For the use case of this code, you are right and as long as we are not doing value replacements, there is no explicit need of the global prefix.
I would even argue in favor of not having the global prefix, since it removes the effect of replacing the value of the global variable and could prevent some unexpected behavior (aka someone adds a line like foo =['this won't do anything']).

if handler not in _command_exit_handlers:
_command_exit_handlers.append(handler)
Comment on lines 89 to 90
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious if this runs successfully in terms of finding the correct reference to the global variable. From my understanding it should fail since _command_exit_handlers is not defined in the function. (But there might be some python juju magic in place to make it kinda work)


Expand All @@ -113,7 +112,6 @@ def main(*, command_name='colcon', argv=None):
:param list argv: The list of arguments
:returns: The return code
"""
global _command_exit_handlers
try:
return _main(command_name=command_name, argv=argv)
except KeyboardInterrupt:
Expand All @@ -126,7 +124,6 @@ def main(*, command_name='colcon', argv=None):


def _main(*, command_name, argv):
global colcon_logger
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in terms of the logger it's possible to remove the global prefix since we never modify the value of it directly but through methods of the same logger.

# default log level, for searchability: COLCON_LOG_LEVEL
colcon_logger.setLevel(logging.WARNING)
set_logger_level_from_env(
Expand Down Expand Up @@ -387,8 +384,6 @@ def create_subparser(parser, cmd_name, verb_extensions, *, attribute):
selected verb
:returns: The special action object
"""
global colcon_logger

# list of available verbs with their descriptions
verbs = []
for name, extension in verb_extensions.items():
Expand Down
Loading