-
Notifications
You must be signed in to change notification settings - Fork 595
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: handle empty string in connector comment padding #3437
Fix: handle empty string in connector comment padding #3437
Conversation
please add a test case |
tests/test_generator.py
Outdated
def test_pad_comment(self): | ||
self.assertEqual(Generator().pad_comment(""), "") | ||
self.assertEqual(Generator().pad_comment(" leading"), " leading ") | ||
self.assertEqual(Generator().pad_comment("trailing "), " trailing ") | ||
self.assertEqual( | ||
Generator().pad_comment(" leading and trailing "), " leading and trailing " | ||
) | ||
self.assertEqual(Generator().pad_comment("no padding"), " no padding ") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we instead test this end-to-end by simply adding an example that fails to roundtrip in pretty.sql?
hm it seems that if moving the check if comment is empty str/None to the method will still cause some issues for some tests:
select x, --
from foo""",
Maybe it's better to add a check to connector_sql and in maybe_comment? I see it's validated like this everywhere else ( if comment: |
TL;DR: Revert my changes in the first place and add if comment check in connector_sql: if comment:
op += f" /*{self.pad_comment(comment)}*/" |
Sure let's do that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, let's add a test in test_transpile.py
and I'll get this in
Problem:
if comment is an empty string pad_comment fails on IndexError on comment[0]. Adding a simple check if comment is not empty