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

PLT-7204 Part 2: Dynamic query #697

Merged
merged 19 commits into from Aug 30, 2023
Merged

Conversation

jhbertra
Copy link
Contributor

  • Adds new library hasql-dynamic-syntax for AST-based dynamic query generation
  • Adds schema types and tables for marlowe schema.
  • Rewrites getPayouts to use hasql-dynamic-syntax

Pre-submit checklist:

  • Branch
    • Tests are provided (if possible)
    • Test report is updated (if relevant)
    • Commit sequence broadly makes sense
    • Key commits have useful messages
    • Relevant tickets are mentioned in commit messages
    • Formatting, PNG optimization, etc. are updated
  • PR
    • Self-reviewed the diff
    • Useful pull request description
      • Review required
      • Substantial changes to code, test, or documentation
      • Change made to Marlowe validator (@bwbush and @palas must be included as reviewers)
      • Review not required
      • Minor changes to non-critical code, documentation, nix derivations, configuration files, or scripts
      • Formatting, spelling, grammar, or reorganization
    • Reviewer requested

@jhbertra jhbertra requested a review from bwbush August 28, 2023 19:01
@jhbertra jhbertra self-assigned this Aug 28, 2023
@jhbertra jhbertra force-pushed the plt-7204-part-2-hasql-dynamic-syntax branch from 0d87189 to 55b1341 Compare August 29, 2023 13:47
@jhbertra jhbertra force-pushed the plt-7204-part-2-hasql-dynamic-syntax branch from 011da4f to 3f8c773 Compare August 30, 2023 13:56
Copy link
Collaborator

@bwbush bwbush left a comment

Choose a reason for hiding this comment

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

This looks good, but there was too much code in the new package for me to review in great detail and there are no tests in the new package's test suite yet.

How were the golden tests generated and checked for correctness? Are they just to detect regressions?

Finally, can we add a changelog entry to runtime for this. Are there no API or breaking changes?

@jhbertra
Copy link
Contributor Author

This looks good, but there was too much code in the new package for me to review in great detail and there are no tests in the new package's test suite yet.
Yeah, I didn't think the new code was particularly high-risk for correctness because it was largely copied-and-pasted from existing high-assurance code (and the types are pretty good here at eliminating errors). At some point, I will write tests for these, for now it's implicitly tested by the integration tests.
How were the golden tests generated and checked for correctness? Are they just to detect regressions?
These are generated in GetPayoutsSpec.hs. Correctness is less the goal as change detection and accurate documentation off what SQL is generated by the code under various configurations. The correctness is checked by the integration tests.
Finally, can we add a changelog entry to runtime for this. Are there no API or breaking changes?
No changelog needed - it's an internal change

@jhbertra jhbertra merged commit b3b2326 into main Aug 30, 2023
6 checks passed
@jhbertra jhbertra deleted the plt-7204-part-2-hasql-dynamic-syntax branch August 30, 2023 16:41
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

2 participants