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

Make Store.namespaces an empty generator #1432

Merged
merged 2 commits into from Oct 11, 2021

Conversation

aucampia
Copy link
Member

90f6fe5 changed

class Store:
    def namespaces(self):
	""" """
	if False:
	    yield None

to

class Store:
    def namespaces(self):
	""" """

This resulted in Store.namespaces no longer being an emtpy generator
by default (see https://stackoverflow.com/q/13243766).

The reason it was removed is because it is unreachable code, however I
did not consider that it would make the function an empty generator.

The reason why if False: yield None made it an empty generator is
because the presence of the yield keyword informs CPython that a
function is a generator:
https://www.python.org/dev/peps/pep-0255/#why-a-new-keyword-for-yield-why-not-a-builtin-function-instead

There are several ways of making an empty generator:

  • if False: yield
  • return and yield
  • yield from ()
  • ... more

Both if False: yield and return; yield results in same bytecode
however, which is that of an empty function, and they are both equally
confusing I would say.

yield from () is somewhat clearer, but the bytecode of the funciton
looks very different from an empty function (see https://stackoverflow.com/a/61496399)

So the best option I see is to just put back what was there and add a
comment and a test to prevent the issue from re-occuring.

Fixes #1431

@aucampia aucampia force-pushed the iwana-20211010T2149-fix_store branch from 443c2f2 to efaf64d Compare October 10, 2021 20:14
@aucampia aucampia force-pushed the iwana-20211010T2149-fix_store branch 2 times, most recently from eef66ca to f4d08cc Compare October 10, 2021 20:31
@aucampia aucampia changed the title Make namespaces a generator by default Make Store.namespaces an empty generator Oct 10, 2021
Not sure what went wrong here but for some reason master branch tries to
export DCMITYPE without also importing it.
90f6fe5 changed

```python
class Store:
    def namespaces(self):
	""" """
	if False:
	    yield None
```

to

```python
class Store:
    def namespaces(self):
	""" """
```

This resulted in `Store.namespaces` no longer being an emtpy generator
by default (see https://stackoverflow.com/q/13243766).

The reason it was removed is because it is unreachable code, however I
did not consider that it would make the function an empty generator.

The reason why `if False: yield None` made it an empty generator is
because the presence of the yield keyword informs CPython that a
function is a generator:
https://www.python.org/dev/peps/pep-0255/#why-a-new-keyword-for-yield-why-not-a-builtin-function-instead

There are several ways of making an empty generator:

- `if False: yield`
- `return` and `yield`
- `yield from ()`
- ... more

Both `if False: yield` and `return`; `yield` results in same bytecode
however, which is that of an empty function, and they are both equally
confusing I would say.

`yield from ()` is somewhat clearer, but the bytecode of the funciton
looks very different from an empty function (see https://stackoverflow.com/a/61496399)

So the best option I see is to just put back what was there and add a
comment and a test to prevent the issue from re-occuring.
@aucampia aucampia force-pushed the iwana-20211010T2149-fix_store branch from f4d08cc to ffff25d Compare October 10, 2021 21:14
@aucampia
Copy link
Member Author

I rebased this onto #1433 to make the test pass, not sure what is going on there exactly though but just ping me for any changes here.

@ashleysommer
Copy link
Contributor

Thanks @aucampia

@aucampia aucampia deleted the iwana-20211010T2149-fix_store branch December 29, 2021 22:06
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.

Restore default implementation of Store.namespaces
2 participants