search

samuelcolvin / python-devtools

Dev tools for python
https://python-devtools.helpmanual.io/
MIT License
985 stars 47 forks source link

Use executing and asttokens #82

Closed alexmojaki closed 3 years ago

alexmojaki commented 3 years ago

This brings in two dependencies to do the heavy lifting for debug:

Both are very well tested and reliable and are used in several other libraries with no issues, including https://github.com/gruns/icecream. You'll note that several test cases no longer give warnings. In particular this fixes #47 and #71.

executing can identify the AST node in almost all situations, which means:

samuelcolvin commented 3 years ago

Thanks a lot, I'll look soon. What is the performance impact?

alexmojaki commented 3 years ago

executing caches the process of identifying the node, so repeated calls are very fast. This code:

from devtools import debug

with debug.timer():
    for i in range(10000):
        debug.format(i + i)

is about 5 times faster now.

alexmojaki commented 3 years ago

The tests are failing because Python 3.8 changed the location for generator expressions, so in older versions parentheses are not included: https://github.com/gristlabs/asttokens/pull/50

Shall I differentiate tests based on version? Or just remove the generator expression from the test?

alexmojaki commented 3 years ago

In case you were put off by the generator issue, I'll clarify. The problem is that in a script like this:

from devtools import debug

debug(x for x in [1, 2])

it's not clear what should be considered the source code of the generator expression. In Python 3.7 you get:

script.py:3 <module>
    x for x in [1, 2]: (
        1,
        2,
    ) (generator)

In 3.8:

script.py:3 <module>
    (x for x in [1, 2]): (
        1,
        2,
    ) (generator)

The only difference is the surrounding parentheses. As far as devtools is concerned, this feels like a pretty unimportant philosophical question. On top of that, putting a generator inside debug() is already a questionable thing to do. So I think it'd be fine to remove this case from the tests.

samuelcolvin commented 3 years ago

Sorry I haven't had time to review this.

On the generators, the behaviour seems absolutely fine. But I'd prefer to keep tests, just have a test for each case and skip them in the other case.

samuelcolvin commented 3 years ago

This looks amazing. I'm so sorry it's taken me so long to get around to reviewing it.

I've had a new job and a new baby, but I know it can still be very annoying when thoughtful and time consuming work like this get's ignored for so long. In short, mea culpa :pray:.

I'm going to make a few tiny changes, merge it and make a new release.

samuelcolvin commented 3 years ago

okay, thanks. I'm trying to see if I can easily remove that function, if not I'll revert.

samuelcolvin commented 3 years ago

hi @alexmojaki,I have a related question for you since you seem to know the python metaprogramming space pretty well - do you know of any faster alternative for highlighing python than pygments?

Much of devtool's poor performance is due to pygments (often >90%). I therefore thinking of building package to wrap https://github.com/trishume/syntect, but I don't want to if something similar already exists.

alexmojaki commented 3 years ago

No I don't know about any alternatives to pygments

samuelcolvin commented 3 years ago

thanks so much for this.

I'll make a new release today.