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

feat: emit_module, emit_generators and emit_async flags #15

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

kevinvalk
Copy link

@kevinvalk kevinvalk commented Aug 22, 2023

In my case, the class wrappers were not nice nor were the usage of generators. Hence I added three flags to change the generated code. During the feature/refactor I noticed it felt weird to generate both async and sync flavors at the same time as most codebases (us including) will use one over the other. Moreover, you can always generate multiple flavors by just running the codegen multiple times.

So what I ended up doing isn to keep the exact same behavior when using emit_sync_querier or and emit_async_querier. If you keep both of these set to false you will go into the "new" flow in which you will only generate one flavor depending on emit_async (so default generates sync code).

All generated code passes all end to end tests without any changes to the tests.

Comment on lines +349 to 383
// TODO: How to better support list comprehension? Maybe add a flag to AST node ForNode and AsyncNode?
_, isCall := n.Body[0].Node.(*ast.Node_Call)
if len(n.Body) == 1 && isCall {
w.print("[\n")
w.printIndent(indent + 1)
w.printNode(node, indent+1)
if i != len(n.Body)-1 {
w.print("\n")
w.printNode(n.Body[0], indent+1)
w.print("\n")
w.printIndent(indent + 1)
if isAsync {
w.print("async ")
}
w.print("for ")
w.printNode(n.Target, indent)
w.print(" in ")
w.printNode(n.Iter, indent)
w.print("\n")
w.printIndent(indent)
w.print("]\n")
} else {
if isAsync {
w.print("async ")
}
w.print("for ")
w.printNode(n.Target, indent)
w.print(" in ")
w.printNode(n.Iter, indent)
w.print(":\n")
for i, node := range n.Body {
w.printIndent(indent + 1)
w.printNode(node, indent+1)
if i != len(n.Body)-1 {
w.print("\n")
}
}
}
Copy link
Author

Choose a reason for hiding this comment

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

@kyleconroy I am not so happy with this list comprehension implementation. How would you like to approach this? Add an extra type to the Python AST protobuf or a flag or?

Copy link
Author

Choose a reason for hiding this comment

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

@kyleconroy now that the ast.proto is available, I can fix this indeed with a flag or a different node. Any preference or leave as is for now?

@kevinvalk kevinvalk marked this pull request as ready for review August 24, 2023 18:10
@kevinvalk
Copy link
Author

@kyleconroy I made some changes to ensure this change is fully backwards compatible. Is there anything blocking or are you not taking any changes for sqlc-gen-python?

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.

1 participant