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 the unexpected error that occurs when an empty control block is used. #387

Closed
wants to merge 2 commits into from

Conversation

cocolato
Copy link
Contributor

@cocolato cocolato commented Feb 4, 2024

Fixes: #146

When the first control block is empty or comment, the correct result will now be rendered.

from mako.template import Template
template = r"""
% if False:
% elif False:
% else:
    test
% endif
"""

t = Template(template)
print(t.render())

Result:


    test

@zzzeek
Copy link
Member

zzzeek commented Feb 4, 2024

cool.

can we add changelog files to these patches? Dont worry about getting them perfect I can fix them up later

example

1d6c58e#diff-22252f506a8da7ab5343802f53f7c35770390f669fe0f73a9e2345f0914ebb98

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 d0a0981 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 d0a0981: https://gerrit.sqlalchemy.org/c/sqlalchemy/mako/+/5148

@cocolato
Copy link
Contributor Author

cocolato commented Feb 4, 2024

Ok, it has been updated.

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 571a73a of this pull request into gerrit so we can run tests and reviews and stuff

@sqla-tester
Copy link
Collaborator

Patchset 571a73a added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/mako/+/5148

Copy link
Member

@zzzeek zzzeek left a comment

Choose a reason for hiding this comment

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

some changes. also inside the commit block, if you can modify this on push (use git commit --amend) , add the fixes line:

Fixes: #146
Closes: #387
Pull-request: https://github.com/sqlalchemy/mako/pull/387
Pull-request-sha: 571a73a1d84ee922b38ce5817a5822fbe0268cd0

mako/codegen.py Outdated
return True
return False

if should_add_pass(node, children):
Copy link
Member

Choose a reason for hiding this comment

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

my comment didnt get transmitted from gerrit.

can we change this, so that we are using the same conditional from the old version, and just this last part with the "for c in children" is moved into a function def _search_for_control_line(). that way the form of the conditional stays the same and we move just that one check.

like this:

if not children or (
    all(
        isinstance(c, (parsetree.Comment, parsetree.ControlLine))
        for c in children
    )
    and all(
        (node.is_ternary(c.keyword) or c.isend)
        for c in children
        if isinstance(c, parsetree.ControlLine)
    )
) or _search_for_control_line(children):
    self.printer.writeline("pass")

Closes: sqlalchemy#387
Pull-request: sqlalchemy#387
Pull-request-sha: f816f1f
@cocolato
Copy link
Contributor Author

cocolato commented Feb 6, 2024

Thanks so much for your help! Everything's been updated now.

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 fb257ff of this pull request into gerrit so we can run tests and reviews and stuff

@sqla-tester
Copy link
Collaborator

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

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.

Michael Bayer (zzzeek) wrote:

code review left on gerrit

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/mako/+/5148

  • /COMMIT_MSG (line 34): Done
  • mako/codegen.py (line 862): Done

@sqla-tester
Copy link
Collaborator

Michael Bayer (zzzeek) wrote:

graet, thanks

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/mako/+/5148

@sqla-tester
Copy link
Collaborator

Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/mako/+/5148 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.

None yet

3 participants