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.

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

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

Merged
merged 2 commits into from
May 25, 2024

Conversation

cottsay
Copy link
Member

@cottsay cottsay commented Feb 22, 2024

I don't see any reason that these statements should be necessary and find their presence confusing.

I'd love to hear feedback either way from any Pythonistas out there who might have some idea why these were added to begin with.

I don't see any reason that these statements should be necessary and
find their presence confusing.
@cottsay cottsay added the help wanted Extra attention is needed label Feb 22, 2024
@cottsay cottsay self-assigned this Feb 22, 2024
Copy link

codecov bot commented Feb 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.23%. Comparing base (db84706) to head (c4ee7d4).

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #621   +/-   ##
=======================================
  Coverage   83.23%   83.23%           
=======================================
  Files          66       66           
  Lines        3842     3842           
  Branches      758      758           
=======================================
  Hits         3198     3198           
  Misses        556      556           
  Partials       88       88           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@christophebedard
Copy link
Contributor

Probably just to make it clear/explicit that they are global variables. I don't see any other reason either.

@cottsay
Copy link
Member Author

cottsay commented Feb 22, 2024

Probably just to make it clear/explicit that they are global variables. I don't see any other reason either.

If we assume that there's no technical value, do you (subjectively) find it to be a helpful pattern?

@christophebedard
Copy link
Contributor

The one upside I can think of is if the code changes and an assignment is added, then it doesn't sneakily turn into a local variable (IIUC), although linters might catch that.

I personally don't have a strong opinion; maybe some more experienced/opinionated people want to chime in.

@cottsay
Copy link
Member Author

cottsay commented Feb 22, 2024

Thanks for your thoughts.

I might suggest converting the variables to use MACRO_CASE to make it more clear that they're globals rather than the global statement.

@@ -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

_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']).

Comment on lines 89 to 90
if handler not in _command_exit_handlers:
_command_exit_handlers.append(handler)
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)

@@ -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.

Copy link
Contributor

@nuclearsandwich nuclearsandwich left a comment

Choose a reason for hiding this comment

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

I went down a rabbit hole the last time I started a review of this PR to try and really grok what python's global keyword does and ultimately I think these are indeed not required. Except as an indicator to the programmer but I don't think language constructs are best used for that over comments.

Programmer’s note: global is a directive to the parser. It applies only to code parsed at the same time as the global statement. In particular, a global statement contained in a string or code object supplied to the built-in exec() function does not affect the code block containing the function call, and code contained in such a string is unaffected by global statements in the code containing the function call. The same applies to the eval() and compile() functions.

@cottsay cottsay removed the help wanted Extra attention is needed label May 24, 2024
@cottsay cottsay marked this pull request as ready for review May 24, 2024 23:29
@cottsay
Copy link
Member Author

cottsay commented May 25, 2024

This has been sitting for a while, and my preference is still to drop the statements, so I'm going to merge this.

Thanks everyone for your thoughts.

@cottsay cottsay merged commit 5ff4911 into master May 25, 2024
36 checks passed
@delete-merged-branch delete-merged-branch bot deleted the cottsay/superfluous-global-statements branch May 25, 2024 00:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants