feat: emit_module, emit_generators and emit_async flags#15
Open
kevinvalk wants to merge 10 commits intosqlc-dev:mainfrom
Open
feat: emit_module, emit_generators and emit_async flags#15kevinvalk wants to merge 10 commits intosqlc-dev:mainfrom
kevinvalk wants to merge 10 commits intosqlc-dev:mainfrom
Conversation
… now generate query code that suites your need
…o configure output files
kevinvalk
commented
Aug 24, 2023
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") | ||
| } | ||
| } | ||
| } |
Author
There was a problem hiding this comment.
@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?
Author
There was a problem hiding this comment.
@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?
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? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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_querieror andemit_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 onemit_async(so default generates sync code).All generated code passes all end to end tests without any changes to the tests.