Skip to content

Conversation

@wchargin
Copy link
Contributor

@wchargin wchargin commented Dec 16, 2020

Summary:
To set a value in an outer scope from within a nested function, we’ve
historically used the pattern of “boxing” the value in a length-1 list:

count_box = [0]

def foo():
    count_box[0] += 1
    ...

foo()
assert count_box[0] == 1

Now that we use Python 3 everywhere, we can avoid the box and just use
the nonlocal keyword instead:

count = 0

def foo():
    nonlocal count
    count += 1
    ...

foo()
assert count == 1

This patch fixes up uses of this pattern, all of which are in tests.

Part of #4488.

Test Plan:
Running git grep -F '_box = [' no longer matches any results.

wchargin-branch: box-to-nonlocal

Summary:
To set a value in an outer scope from within a nested function, we’ve
historically used the pattern of “boxing” the value in a length-1 list:

```python
count_box = [0]

def foo():
    count_box[0] += 1
    ...

foo()
assert count_box[0] == 1
```

Now that we use Python 3 everywhere, we can avoid the box and just use
the `nonlocal` keyword instead:

```python
count = 0

def foo():
    nonlocal count
    count += 1
    ...

foo()
assert count == 1
```

This patch fixes up uses of this pattern, all of which are in tests.

Test Plan:
Running `git grep -F '_box = ['` no longer matches any results.

wchargin-branch: box-to-nonlocal
wchargin-source: 31f133d15c863c174d6f5ece384fbc48a7039503
Copy link
Contributor

@nfelt nfelt left a comment

Choose a reason for hiding this comment

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

Oh python :/ Thanks for the cleanup. If you wanna create an issue for "thank goodness we finally can stop caring about Python 2" and log cleanup like this to it you might as well get credit for it.

@wchargin wchargin mentioned this pull request Dec 16, 2020
10 tasks
@wchargin
Copy link
Contributor Author

Sure. Credit attribution aside, that will be a nice place to keep a
to-do list of “changes that I want to make in low-traffic periods to
avoid merge conflicts”, like “remove all __future__ imports”. #4488

@wchargin wchargin merged commit 48c0a2d into master Dec 16, 2020
@wchargin wchargin deleted the wchargin-box-to-nonlocal branch December 16, 2020 23:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants