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

fix bpo-31183: allowing dis to disassemble async generator and coroutine objects #3077

Merged
merged 15 commits into from
Aug 18, 2017

Conversation

syncosmic
Copy link
Contributor

@syncosmic syncosmic commented Aug 11, 2017

I've tried to stay as close to the resolution of issue-21947 as possible; this just extends that to two new types of objects: async generator objects and coroutine objects.

This shouldn't have any backwards-compatibility concerns unless someone was relying on dis breaking on one of these.

The patch to dis is trivial; adding a test and suppressing the warning was slightly less so. As mentioned in comments, I pulled the coroutine testing approach from Trio (except for inlining the gc_collect_harder function).

Thanks to Luciano Ramalho for the substantive solution!

https://bugs.python.org/issue31183

Extended issue 21947 to handle async generator and coroutine objects.
Added unit tests based on issue 21947 for async generator and coroutine
objects. This required two additional steps: borrowing (and slightly adapting) a context manager written for Trio in order to suppress a leaking-coroutine warning, and fixing a latent bug in test_source_line_in_disassembly (which assumed the source code line would never be > 999. (For consistency, I used the same approach for the explicit line number, even though the number chosen (350) doesn't require it).
@the-knights-who-say-ni
Copy link

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

Thanks again to your contribution and we look forward to looking at it!

@syncosmic syncosmic changed the title fix issue 31183: allowing dis to disassemble async generator and coroutine objects fix bpo-31183: allowing dis to disassemble async generator and coroutine objects Aug 11, 2017
@syncosmic
Copy link
Contributor Author

Ni! Ni! Ni! I just signed the CLA.

Copy link
Contributor

@ncoghlan ncoghlan left a comment

Choose a reason for hiding this comment

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

Overall, this looks good to me, just one minor tweak suggested for a string method call, and one open question regarding the test helper (where I'd like feedback from @serhiy-storchaka and @1st1 on where we put it)

# Use the line in the source code
actual = dis.Bytecode(simple).dis()[:3]
# Use the line in the source code (split extracts the line no)
actual = dis.Bytecode(simple).dis().split(" ")[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we only want the first entry, .partition(" ")[0] would be a better option here (it doesn't really matter all that much, given that disassembly lines are short, but we may as well avoid pointlessly splitting the rest of the line up into fields when we don't care about them).

Copy link
Member

Choose a reason for hiding this comment

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

Or just use [:3].

Or use [:4] and add a space to the expected value (this is more robust).

actual = dis.Bytecode(simple).dis()[:4]
expected = "{:>3} ".format(simple.__code__.co_firstlineno)

Copy link
Contributor Author

@syncosmic syncosmic Aug 12, 2017

Choose a reason for hiding this comment

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

[:3] raises now (since this is the patch that puts the line over 999) and [:4] would raise if it's ever driven above 9999; that would be a complex test suite (!) but I'll go with .partition (good call on that vs. .split; thanks!)

Copy link
Member

Choose a reason for hiding this comment

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

Maybe use [:len(expected)]?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

expected hasn't been defined yet, so we'd have to change the order (raising the point that we would make an order-independent setup order-dependent). It also breaks consistency with the second comparison; closest w/o refactoring would to be [:len("350")], repeating the magic number a third time, just as [:3] encodes the length of the magic number.

If it were up to me I'd just pick .partition(" ")[0]; in addition to the control flow simplicity and consistency I think it's clearer (we're splitting the string up and picking part of it). @serhiy-storchaka , do you feel really strongly about this? @ncoghlan , do you prefer [:len(expected)] (w/ [:len('350')] ) as well?

Copy link
Member

Choose a reason for hiding this comment

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

The only problem is more complicated code and repeated " ". It may takes few minutes for figuring out why the code is written in this way. But this may be subjective. If this code looks good for other reviewers I have no objections.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To my eye, it's clearer this way, since the focus is on what we're doing to the dis line and the verbs are all simple: strip whitespace from the left, split on whitespace and pick the first region.

The main clarity improvement would seem to be refactoring out an extract_line_num method, but per my commit comment that seems like overkill in test code, and I'm not sure what the best place to put it would be.

Others?

Copy link
Contributor

Choose a reason for hiding this comment

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

@syncosmic I think you can simplify the call by just stripping all surrounding whitespace rather than specifically leading spaces:

actual = dis.Bytecode(simple).dis().strip().partition(" ")[0]

The fact there's only one string literal in there then more clearly conveys "partition the text on the first internal space".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, that is better. Seeing it written out w/your explanation I get @serhiy-storchaka 's point about not doubling the literal. I'll tweak that and fix the docstring now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I did this but in two lines each. It's clearer, takes the comment better, and avoids having to do an ugly line continuation in the second case.



@contextlib.contextmanager
def ignore_coroutine_never_awaited_warnings():
Copy link
Contributor

Choose a reason for hiding this comment

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

@serhiy-storchaka, @1st1 Review question here - for a dis module test, we want to create a coroutine that we never await, so @syncosmic has suggested adding this helper context manager.

I like the idea, but I'm wondering if we should put it in test.support, rather than inline in the dis module tests.

Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

Isn't it easier to just call coro.send(None) to silence the warning?

Copy link
Member

Choose a reason for hiding this comment

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

That's what I did in test_coroutines IIRC.

Copy link
Contributor

Choose a reason for hiding this comment

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

@1st1 Good point, although coro.close() is the appropriate method call that won't raise StopIteration in the test case.

I'll add a comment to the relevant line in the test.

@bedevere-bot
Copy link

A Python core developer, ncoghlan, has requested some changes be
made to your pull request before we can consider merging it. If you
could please address their requests along with any other requests in
other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment
on this pull request containing the phrase I didn't expect the Spanish Inquisition!
I will then notify ncoghlan along with any other core developers
who have left a review that you're ready for them to take another look
at this pull request.

Lib/dis.py Outdated
@@ -107,13 +112,19 @@ def pretty_flags(flags):
return ", ".join(names)

def _get_code_object(x):
"""Helper to handle methods, functions, generators, strings and raw code objects"""
"""Helper to handle methods, functions, generators,
Copy link
Member

Choose a reason for hiding this comment

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

Even though this is a "private" method, it should follow the guidelines for docstings. The first line should be a complete sentence shorter than 72 chars describing the function.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe just turn this docstring in a comment? But we should solve the similar issue with the dis() docstring.

Copy link
Contributor Author

@syncosmic syncosmic Aug 12, 2017

Choose a reason for hiding this comment

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

Hm. What about """Helper to handle methods, raw or attributive code objects, and strings""" ? That wouldn't need to be changed if new objects with .??_code attributes are invented.

@@ -535,6 +579,17 @@ def test_disassemble_generator(self):
gen_disas = self.get_disassembly(_g(1)) # Disassemble generator itself
self.assertEqual(gen_disas, gen_func_disas)

def test_disassemble_async_generator(self):
agen_func_disas = self.get_disassembly(_ag) # Disassemble async generator function
Copy link
Member

@1st1 1st1 Aug 12, 2017

Choose a reason for hiding this comment

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

This line is longer than 79 chars. Please follow PEP8.

Copy link
Contributor Author

@syncosmic syncosmic Aug 12, 2017

Choose a reason for hiding this comment

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

Apologies! Will fix. Thanks.

self.assertEqual(agen_disas, agen_func_disas)

def test_disassemble_coroutine(self):
coro_func_disas = self.get_disassembly(_co) # Disassemble coroutine function
Copy link
Member

@1st1 1st1 Aug 12, 2017

Choose a reason for hiding this comment

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

Another nit: two spaces between code and # -- people who use linters will see this line highlighted in their editors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

More apologies! Will also fix.

@1st1
Copy link
Member

1st1 commented Aug 12, 2017

LGTM, aside a few nits that ideally should be fixed.

Copy link
Contributor

@ncoghlan ncoghlan left a comment

Choose a reason for hiding this comment

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

A couple more comments:

  • one substantive one to avoid the runtime warning entirely by explicitly closing the coroutine
  • one cosmetic one to tweak the sample coroutine to be one that actually runs in addition to compiling



@contextlib.contextmanager
def ignore_coroutine_never_awaited_warnings():
Copy link
Contributor

Choose a reason for hiding this comment

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

@1st1 Good point, although coro.close() is the appropriate method call that won't raise StopIteration in the test case.

I'll add a comment to the relevant line in the test.

yield x

async def _co(x):
await _ag(x)
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking more closely, this should really be:

async def _co(x):
    async for item in _ag(x):
        pass

The current code will happily compile (and hence disassemble), but emits TypeError: object async_generator can't be used in 'await' expression if you actually try to run it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Excellent point. Thanks.

def test_disassemble_coroutine(self):
coro_func_disas = self.get_disassembly(_co) # Disassemble coroutine function
with ignore_coroutine_never_awaited_warnings():
coro_disas = self.get_disassembly(_co(1)) # Disassemble coroutine itself
Copy link
Contributor

Choose a reason for hiding this comment

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

As per @1st1's comments above, a simpler way of avoiding the warning here is to explicitly close the coroutine:

coro = _co(1)
coro.close()
coro_disas = self.get_disassembly(coro)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Much better!

@bedevere-bot
Copy link

A Python core developer, ncoghlan, has requested some changes be
made to your pull request before we can consider merging it. If you
could please address their requests along with any other requests in
other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment
on this pull request containing the phrase I didn't expect the Spanish Inquisition!
I will then notify ncoghlan along with any other core developers
who have left a review that you're ready for them to take another look
at this pull request.

Lib/dis.py Outdated
@@ -32,7 +32,8 @@ def _try_compile(source, name):
return c

def dis(x=None, *, file=None, depth=None):
"""Disassemble classes, methods, functions, generators, or code.
"""Disassemble classes, methods, functions, generators,
Copy link
Member

Choose a reason for hiding this comment

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

The below comment of @1st1 is applicable to this function.

Copy link
Contributor Author

@syncosmic syncosmic Aug 12, 2017

Choose a reason for hiding this comment

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

I think this one is harder to get right in 72 characters, though (and as you point out below we can't just substitute a comment since it's public).

Maybe better but possibly confusing: Disassemble classes, methods, functions, function-like objects, or code.

Is there a better generic phrase for generators, asynchronous generators, and coroutines? It seems wrong to mention generators explicitly but not the other two, but maybe that's the cost of living. I do think we need to mention functions explicitly in the top-level public docstring (unlike _get_code_object, where a mechanical definition may be clearer).

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps:

"""Disassemble classes, functions, and other compiled objects (e.g. generators)"""

Copy link
Contributor Author

@syncosmic syncosmic Aug 12, 2017

Choose a reason for hiding this comment

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

Best yet, but I regret dropping code and methods. How about this:

Disassemble classes, methods, functions, other compiled objects, or code.

With no argument, disassemble the last traceback.

Compiled objects currently include generator objects, async generator
objects, and coroutine objects, all of which store their code object
in a special attribute.

Lib/dis.py Outdated
@@ -46,6 +47,10 @@ def dis(x=None, *, file=None, depth=None):
x = x.__code__
if hasattr(x, 'gi_code'): # Generator
x = x.gi_code
if hasattr(x, 'ag_code'): # Async generator
Copy link
Member

Choose a reason for hiding this comment

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

Since generator, async generator and coroutine can't be a value of function's __code__ attribute, I think it would be better to use elif instead of if in tests for generator, async generator and coroutine.

Copy link
Contributor Author

@syncosmic syncosmic Aug 12, 2017

Choose a reason for hiding this comment

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

Agreed--thanks!

This passage has some slightly tricky in-place substitutions which will be harder to read with mixed if and if-elif passages (basically, 46 and line 54 can't be elif, but it sort of looks as though they could be). I'd propose a space after line 43 to distinguish the None guard from the code-object indirection(s), and a space after line 53 to distinguish the actual application on x. I think that's within PEP8's' "sparingly" requirement.

This is going a bit outside of the patch. Maybe it's a separate, very minor pull request?

Lib/dis.py Outdated
@@ -107,13 +112,19 @@ def pretty_flags(flags):
return ", ".join(names)

def _get_code_object(x):
"""Helper to handle methods, functions, generators, strings and raw code objects"""
"""Helper to handle methods, functions, generators,
Copy link
Member

Choose a reason for hiding this comment

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

Maybe just turn this docstring in a comment? But we should solve the similar issue with the dis() docstring.

# Use the line in the source code
actual = dis.Bytecode(simple).dis()[:3]
# Use the line in the source code (split extracts the line no)
actual = dis.Bytecode(simple).dis().split(" ")[0]
Copy link
Member

Choose a reason for hiding this comment

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

Or just use [:3].

Or use [:4] and add a space to the expected value (this is more robust).

actual = dis.Bytecode(simple).dis()[:4]
expected = "{:>3} ".format(simple.__code__.co_firstlineno)

@ncoghlan
Copy link
Contributor

Beyond the code changes requested, a couple of other items to note:

The initial version broke if the number of digits in the line where simple() was defined increased beyond 3, which happened during the patch. After discussion in the first PR, I went with what seemed to be the clearest general method and kept consistency between the actual case and the explicit case (although the explicit case would only break if the magic number changed). This should now allow arbitrary whitespace on either side and any number of digits in the line number. The .lstrip().partition() chain is repeated, obviously, but it didn't seem worth refactoring into a function since this is "just" test code.
This isn't DRY and should probably be refactored so that dis() calls _get_code_object() (or so that both call a simpler helper method) but that's a different patch.
@syncosmic
Copy link
Contributor Author

Resubmitted--I think this addresses everything.

I didn't expect the Spanish Inquisition! :)

@bedevere-bot
Copy link

Nobody expects the Spanish Inquisition!

@ncoghlan, @1st1, @1st1, @1st1, @1st1, @1st1, @ncoghlan, @serhiy-storchaka, @ncoghlan, @serhiy-storchaka, @serhiy-storchaka: please review the changes made to this pull request.

@syncosmic
Copy link
Contributor Author

Looks like bedevere-bot needs set, not list...

Copy link
Contributor

@ncoghlan ncoghlan left a comment

Choose a reason for hiding this comment

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

Some minor notes inline that I think would be improvements to docs and comments, but nothing that I'd consider a blocker to merging this.

However, holding off on formal approval until @1st1 and @serhiy-storchaka have had a chance to comment further :)

@@ -93,6 +93,9 @@ code.
Return a formatted multi-line string with detailed information about the
code object, like :func:`code_info`.

.. versionchanged:: 3.7
This can now handle asynchronous generator and coroutine objects.
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor nit: "coroutine and asynchronous generator" is slightly easier to parse when reading (since the scope of the adjective is unambiguous)

Lib/dis.py Outdated
@@ -32,25 +32,30 @@ def _try_compile(source, name):
return c

def dis(x=None, *, file=None, depth=None):
"""Disassemble classes, methods, functions, generators,
asynchronous generators, coroutines, or code.
"""Disassemble classes, methods, functions, other compiled objects, or code.
Copy link
Contributor

Choose a reason for hiding this comment

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

Using both other and or here looks odd to me. Perhaps:

"""Disassemble classes, methods, functions, and other compiled objects.

I don't think it's necessary to specifically mention code objects in this first line - the fact that "other compiled objects" will include the output of a compile() call is a fairly obvious assumption.

Lib/dis.py Outdated

Compiled objects currently include generator objects, async generator
objects, and coroutine objects, all of which store their code object
in a special attribute.
Copy link
Contributor

Choose a reason for hiding this comment

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

Reviewing this and thinking about how modules and classes are handled made me realise that there's now a discrepancy between the types that are accepted as arguments to dis, and those that will be disassembled when present as instance, class or module attributes.

Specifically, the filter for the __dict__ handling case is defined in terms of isinstance and the _have_code type tuple, while dis() itself works entirely by checking for particular attributes.

Resolving that is out of scope for this PR, so I've filed a new issue to cover it: https://bugs.python.org/issue31197

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, and the refactoring you propose could probably also DRY up the repetition in dis and _get_code_object as a side bonus.

x = x.__code__
if hasattr(x, 'gi_code'): # Generator
elif hasattr(x, 'gi_code'): #...a generator object, or
Copy link
Contributor

Choose a reason for hiding this comment

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

While I don't think we should hold up this PR for it, this does make we wonder if we should just be exposing a __code__ attribute on all of these types.

Copy link
Contributor Author

@syncosmic syncosmic Aug 14, 2017

Choose a reason for hiding this comment

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

This struck me as well, so I did a little digging on the history and context. It looks as though gi_code was added to generators in bpo-1473257. At this time, function bytecode was still stored in f.func_code, so gi_code was a clear analogy.

Then func_code was changed in 3.0 to __code__ as part of the f.func_X to f.__X__ renaming. The 3.0 whatsnew explained that the purpose of this change was to "free up [the f.func_X] names in the function attribute namespace for user-defined attributes". But this wasn't done for the analogous code object in generators. On a quick look, I didn't find any discussion of this at the time, but that doesn't mean there wasn't any.

It seems, then, that there are two questions now:

  1. Should the code objects in these three types also be dundered for namespace reasons?
  2. Should they share a common name with functions or is there a reason why (following bpo-1473257) they should have different names (whether or not dundered)?

1 seems easier to answer, although that kind of breaking change was probably more in the spirit of 3.0 than 3.7. I have a couple of dim thoughts on 2 but I'll save them for possible later discussion elsewhere.

This also raises the question of whether at least some of the other [gi|ag|cr]_X attributes should be dundered and/or homogenized.

Copy link
Contributor

Choose a reason for hiding this comment

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

At least some of the others are deliberately different because they refer to slightly different things (e.g. cr_await vs gi_yieldfrom).

Anyway, I've added a note about that to the refactoring issue: https://bugs.python.org/issue31197#msg300291

gen_func_disas = self.get_disassembly(_g) # Disassemble generator function
gen_disas = self.get_disassembly(_g(1)) # Disassemble generator itself
gen_func_disas = self.get_disassembly(_g) # Generator function
gen_disas = self.get_disassembly(_g(1)) # Generator object
Copy link
Contributor

Choose a reason for hiding this comment

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

# Generator iterator to be precise

agen_func_disas = self.get_disassembly(_ag) # Disassemble async generator function
agen_disas = self.get_disassembly(_ag(1)) # Disassemble async generator itself
agen_func_disas = self.get_disassembly(_ag) # Async generator function
agen_disas = self.get_disassembly(_ag(1)) # Async generator object
Copy link
Contributor

Choose a reason for hiding this comment

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

# Async generator iterator

@brettcannon
Copy link
Member

Thanks for everyone being patient with the quirks of Bedevere's stage labeling; this PR gets to have the "honour" of being the first one to go from change request to re-review request since going live.

I've opened python/bedevere#38 and python/bedevere#39 to address some of the issues.

@syncosmic
Copy link
Contributor Author

@brettcannon : Honoured! :)

@syncosmic
Copy link
Contributor Author

@ncoghlan : I know we're still waiting on @1st1 and @serhiy-storchaka , but I went ahead and committed your doc and # suggestions. Thanks for those!

Copy link
Contributor

@ncoghlan ncoghlan left a comment

Choose a reason for hiding this comment

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

LGTM now, so unless @1st1 or @serhiy-storchaka chime in with further requests for changes, I'll merge this tomorrow.

Lib/dis.py Outdated

With no argument, disassemble the last traceback.
With no argument, disassemble the last traceback.
Copy link
Member

Choose a reason for hiding this comment

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

Align the docstring at the left side of """. Seems this is never specified explicitly (likely because nobody could think of other way of formatting), but all multiline docstrings are aligned at the left side of """.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Happy to change it, but this convention is inconsistent even within dis.py: compare e.g. _try_compile, Instruction and _get_name_info, which align to the right of """, with _disassemble, get_instructions, and get_instructions_bytes, which use the alignment you prefer. I was matching _try_compile since it's directly above.

git blame sheds no light on this that I can see, since inconsistent alignments were used even within the same patch.

Also, I just confirmed that after applying trim from PEP 257 (in 3.x you have to manually add a dummy maxint to sys), all three docstrings below are equivalent. (PEP 257 only mentions foo and bar).

def foo():
    """A multi-line
    docstring.
    """

def bar():
    """
    A multi-line
    docstring.
    """
    
def baz():
    """A multi-line
       docstring.
    """

Copy link
Contributor

Choose a reason for hiding this comment

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

We are sometimes inconsistent, but as @serhiy-storchaka notes, the preferred layouts are the ones that align with the opening triple-quote - the last option isn't shown in PEP 257 because we're not supposed to use it :)

The inconsistent ones just get left alone once they're in, since we don't typically do formatting-only edits - we're more likely to fix the indentation as part of a change that needed to update the affected docstring anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. Thanks! Makes sense.

# Use the line in the source code
actual = dis.Bytecode(simple).dis()[:3]
# Use the line in the source code (split extracts the line no)
actual = dis.Bytecode(simple).dis().split(" ")[0]
Copy link
Member

Choose a reason for hiding this comment

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

The only problem is more complicated code and repeated " ". It may takes few minutes for figuring out why the code is written in this way. But this may be subjective. If this code looks good for other reviewers I have no objections.

@ncoghlan ncoghlan merged commit fe2b56a into python:master Aug 18, 2017
@ncoghlan
Copy link
Contributor

ncoghlan commented Aug 18, 2017

And merged! Thanks @syncosmic for the patch, and @1st1 and @serhiy-storchaka for your comments :)

Just noting I filed https://bugs.python.org/issue31230 to cover the idea of adding a __code__ attribute to the affected types (including generator iterators), which would make it possible to drop the special-casing in dis.

@syncosmic
Copy link
Contributor Author

Great! Thanks, all! @ncoghlan , I'll follow bpo-31230 with interest :)

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.

None yet

8 participants