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 support comprehensions inside functions when use strict_undefined… #399

Closed
wants to merge 2 commits into from

Conversation

sgranjoux
Copy link

… flag

Fixes: #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.

@zzzeek zzzeek requested a review from sqla-tester May 13, 2024 13:04
Copy link
Collaborator

@sqla-tester sqla-tester left a comment

Choose a reason for hiding this comment

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

OK, this is sqla-tester setting up my work on behalf of zzzeek to try to get revision a52802b of this pull request into gerrit so we can run tests and reviews and stuff

@sqla-tester
Copy link
Collaborator

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

@zzzeek
Copy link
Member

zzzeek commented May 13, 2024

thanks!

@sgranjoux
Copy link
Author

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
Copy link
Author

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

@zzzeek
Copy link
Member

zzzeek commented May 13, 2024

pep8 failures you can address if you install pre commit:

pre-commit install
pre-commit run

@zzzeek
Copy link
Member

zzzeek commented May 13, 2024

also sure changeset file would be very helpful thanks!

@sgranjoux
Copy link
Author

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

@zzzeek
Copy link
Member

zzzeek commented May 13, 2024

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,

Sébastien Granjoux added 2 commits May 13, 2024 17:19
… flag

Fixes: sqlalchemy#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.
Copy link
Collaborator

@sqla-tester sqla-tester left a comment

Choose a reason for hiding this comment

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

OK, this is sqla-tester setting up my work on behalf of zzzeek to try to get revision d708e27 of this pull request into gerrit so we can run tests and reviews and stuff

@sqla-tester
Copy link
Collaborator

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

@zzzeek
Copy link
Member

zzzeek commented May 13, 2024

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

@sqla-tester
Copy link
Collaborator

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

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.

Accessing an attribute or an index in a dictionnary comprehension trigger an error with the mako version 1.3.3
3 participants