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

Use the library executing #33

Merged
merged 10 commits into from
Oct 25, 2019
Merged

Use the library executing #33

merged 10 commits into from
Oct 25, 2019

Conversation

alexmojaki
Copy link
Collaborator

Hi!

I wanted to integrate this library's functionality into my own project, so I looked into how it worked, and discovered that in many cases it didn't. Here's a quick script showing various cases in which it fails. They all work after this PR.

from icecream import ic


def main():
    x = 1
    (
        ic(x))

    if ic(x):
        pass

    ic([ic(x)])
    {ic(x) for x in ic([1, 2])}
    list(ic(x) for x in [1, 2])
    (lambda: ic(x))()
    return ic(x)


main()

I tried to fix the algorithm, but slowly discovered more and more that it was fundamentally broken. In the end all that's left is the general idea of analysing the bytecode and using frame.f_lasti. But I want to emphasise that even this was a big leap, and what you accomplished was very impressive. I didn't know that this was a solvable problem. Your code inspired me and set me in the right direction to create something very cool with broad potential applications.

The new algorithm now lives in a new general purpose little package I've created called executing. It finds the AST node corresponding to the ic() call. Translating that into source code is done with asttokens, which is the best method I know of. Unfortunately it comes with a couple of small problems:

  1. Python 3.4 is not supported.
  2. The source code for tuples doesn't include surrounding parentheses, e.g. ic((1, 2)) just gives 1, 2 as the source, which is causing some failures. If you want to fix this, take a look at Include parentheses around expressions in token range gristlabs/asttokens#11

There's also some failures because collapseWhitespaceBetweenTokens doesn't seem to be doing what the name suggests. Personally I don't think the displayed source code should be modified anyway. If you still want to do that, you'll have to figure out how.

Some other perks:

  1. Caching make repeated calls very fast.
  2. Files with encoding declarations are decoded correctly.
  3. It works in IPython shells and notebooks.
  4. icecream.ic() works. In fact ic can be the result of any expression.

@gruns
Copy link
Owner

gruns commented Jul 10, 2019

Awesome work. Will dig into this PR shortly!

@gruns
Copy link
Owner

gruns commented Jul 13, 2019

Python 3.4 is not supported.

What breaks with Python 3.4?

The source code for tuples doesn't include surrounding parentheses,
e.g. ic((1, 2)) just gives 1, 2 as the source, which is causing some
failures. If you want to fix this, take a look at gristlabs/asttokens#11

Yep. Same behavior manually detected and accounted for by icecream in
extractArgumentsFromCallStr(). See

Could still detect and do the same with extraction.py.

There's also some failures because collapseWhitespaceBetweenTokens doesn't
seem to be doing what the name suggests. Personally I don't think the
displayed source code should be modified anyway. If you still want to do that,
you'll have to figure out how.

Displayed source should be normalized. This

ic(  a   +   b     )

is best printed as

ic| a + b: 1

not

ic|   a   +   b     : 1

Source code alignment and spacing, which may be arbitrary, shouldn't affect
debugging legibility and readability. ic() should better source legibility
where possible.

@alexmojaki
Copy link
Collaborator Author

There are some tests in asttokens that fail in 3.4, I don't remember which. I'm guessing it'll work most of the time but sometimes may silently do the wrong thing.

If I write:

ic(foo(
    bar=thing,
    baz=qux([
        x + y
        for x in range(n)
        for y in range(x)
    ])
))

and I see

ic| foo(bar=thing, baz=qux([x + y for x in range(n) for y in range(x)]),)
  1. It might actually take me a moment to realise that those two go together (assuming I had many ic() calls in my code at the time). I think it's better to let the user recognise source code at a glance just because it's familiar than make them reread it.
  2. I'm gonna be annoyed because the latter is much harder to read and understand. I don't know why you would assume that collapsing whitespace is always better than the choices I intentionally made.

Your example with ic( a + b ) is very weird and contrived. No one does that. And if they did, there might be a reason for it.

In any case, like I said, it's up to you to figure out how to make whitespace and parentheses look how you want.

@alexmojaki alexmojaki merged commit ba65704 into gruns:master Oct 25, 2019
@gruns
Copy link
Owner

gruns commented Oct 26, 2019

Huge thank you @alexmojaki for all your hard work to merge executing into icecream.

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