colcon / colcon-core

Command line tool to build sets of software packages
http://colcon.readthedocs.io
Apache License 2.0
99 stars 44 forks source link

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

Closed cottsay closed 2 months ago

cottsay commented 5 months ago

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.

codecov[bot] commented 5 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

christophebedard commented 5 months ago

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

cottsay commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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.

cottsay commented 2 months ago

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.