Fix support comprehensions inside functions when use strict_undefined…

sqlalchemy / mako

Mako Templates for Python

https://www.makotemplates.org

MIT License

353 stars 60 forks source link

Fix support comprehensions inside functions when use strict_undefined… #399

Closed sgranjoux closed 3 months ago

sgranjoux commented 3 months ago

… flag

Fixes: https://github.com/sqlalchemy/mako/issues/398

The element or key and value used respectively in list/set comprehension and dictionary comprehension cannot be used to declare identifiers so no need to visit them.

sqla-tester commented 3 months ago

New Gerrit review created for change a52802b935e038a4651dbc0a06384eb7471793bd: https://gerrit.sqlalchemy.org/c/sqlalchemy/mako/+/5322

zzzeek commented 3 months ago

thanks!

sgranjoux commented 3 months ago

I have added an test failing with version 1.3.3.

I think the first issue is that an AST node representing a dictionnary comprehension hasn't a attribute elt but key and value instead so the line https://github.com/sgranjoux/mako/blob/af80cbd71c2e68954310c4b06f4514f6718d2575/mako/pyparser.py#L107 is wrong. It's should be at least node.key instead of node.elt.

But this doesn't fix the initial issue if node.elt, node.key or node.value is not a Name but a Subscript like in my new test or an BinOp like in the test already in the regression. I think we just need to skip these nodes.

I was just an user of the mako library and I have spent just a few hours trying to fix this issue. @cocolato can you take look at this?

It's my first pull request. I have just seen that some things are failing, I will check that.

sgranjoux commented 3 months ago

I don't know why 2 of the checks above are failing. Should I add a doc/build/unreleased/398.rst file?

zzzeek commented 3 months ago

pep8 failures you can address if you install pre commit:

pre-commit install
pre-commit run

zzzeek commented 3 months ago

also sure changeset file would be very helpful thanks!

sgranjoux commented 3 months ago

I have added a changeset file and the commit hooks but they don't report any failures.

zzzeek commented 3 months ago

oh, this is a new thing in flake8 that we are disabling, sorry

please add this

diff --git a/setup.cfg b/setup.cfg
index f643d01..ecbbe0f 100644
--- a/setup.cfg
+++ b/setup.cfg
@@ -89,7 +89,7 @@ show-source = true
 enable-extensions = G
 # E203 is due to https://github.com/PyCQA/pycodestyle/issues/373
 ignore =
-    A003,
+    A003,A005
     D,
     E203,E305,E711,E712,E721,E722,E741,
     N801,N802,N806,

sqla-tester commented 3 months ago

Patchset d708e27f988da7e1923a1a9dab90a0884ef84bbb added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/mako/+/5322

zzzeek commented 3 months ago

i dont get any ping when code is updated.....running again

sqla-tester commented 3 months ago

Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/mako/+/5322 has been merged. Congratulations! :)