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

lazy: check sentinel identity with is, not == #1980

Merged
merged 2 commits into from
Mar 8, 2019

Conversation

wchargin
Copy link
Contributor

@wchargin wchargin commented Mar 7, 2019

Summary:
This is not likely to be a problem in practice, but we might as well do
the safe thing when comparing to a sentinel.

Test Plan:
Unit test added, for good measure.

wchargin-branch: lazy-sentinel-is

Summary:
This is not likely to be a problem in practice, but we might as well do
the safe thing when comparing to a sentinel.

Test Plan:
Unit test added, for good measure.

wchargin-branch: lazy-sentinel-is
@wchargin wchargin requested a review from nfelt March 7, 2019 23:13
# This would fail if the implementation of `_memoize` used `==`
# rather than `is` to check for the sentinel value.
class EqualToEverything(object):
init_count = 0
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 a little better to just have a counter within foo() itself? Since what we are really tracking is that foo() should be called just once, regardless of what it does. That way we separate concerns so that EqualToEverything is only doing what it says on the label and not also being a counter.

This does require an indirection to get around python read-only closures, e.g. foo.counter = getattr(foo, 'counter', 0) + 1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I originally had the standard count_box = [0] trick, which I can
reinstate. You can’t just store the value on foo itself because that
will refer to the wrapped LazyModule; i.e., using

    @lazy.lazy_load("foo")
    def foo():
      foo.counter = getattr(foo, "counter", 0) + 1
      return EqualToEverything()

raises “Circular import when resolving LazyModule 'foo'”. You can get around this with

    @lazy.lazy_load("foo")
    def foo():
      foo.counter += 1
      return EqualToEverything()
    foo.counter = 0

but given that it’s this subtle I’m inclined to stick to the much
simpler close-over-[0], which is at least somewhat idiomatic.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, I was being too clever. The boxing singleton list is fine, sure.

wchargin-branch: lazy-sentinel-is
@wchargin wchargin merged commit 4857d1b into master Mar 8, 2019
@wchargin wchargin deleted the wchargin-lazy-sentinel-is branch March 8, 2019 00:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants