Skip to content

Support parsing multiple SQL statements in sql2pgroll.Convert #690

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

Merged
merged 2 commits into from
Feb 19, 2025

Conversation

kvch
Copy link
Contributor

@kvch kvch commented Feb 18, 2025

This PR introduces support for translating multiple SQL statements to pgroll operations in sql2pgroll.Convert.

This PR required by the upcoming improvements to support reading complete SQL migration
files produced by ORMs.

@kvch kvch marked this pull request as draft February 18, 2025 11:24
@kvch kvch force-pushed the feature-return-migration-on-convert branch 3 times, most recently from b92cd27 to 235b2f7 Compare February 18, 2025 11:47
@kvch kvch marked this pull request as ready for review February 18, 2025 11:52
@kvch kvch requested a review from andrew-farries February 18, 2025 11:52
@kvch kvch force-pushed the feature-return-migration-on-convert branch from 235b2f7 to 25d7008 Compare February 18, 2025 15:39
Copy link
Collaborator

@andrew-farries andrew-farries left a comment

Choose a reason for hiding this comment

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

Left some comments.

More generally, could the signature of Convert remain as returning a slice of operations? The process of putting them into a Migration and generating a name for the migration seems like functionality that should be in whatever is calling Convert, not in Convert itself.

@kvch kvch force-pushed the feature-return-migration-on-convert branch from 25d7008 to 54bed01 Compare February 19, 2025 14:02
@kvch
Copy link
Contributor Author

kvch commented Feb 19, 2025

@andrew-farries I have reworked the pull request to keep the signature of Convert. I added more tests to cover cases like creating functions, or running statements with comments as well.

@kvch kvch requested a review from andrew-farries February 19, 2025 14:05
@kvch kvch changed the title Change sql2pgroll.Convert to return Migration and parse multiple statements Support parsing multiple SQL statements in sql2pgroll.Convert Feb 19, 2025
@kvch kvch force-pushed the feature-return-migration-on-convert branch from 54bed01 to 0e3748f Compare February 19, 2025 14:13
Copy link
Collaborator

@andrew-farries andrew-farries left a comment

Choose a reason for hiding this comment

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

Looks good 👍

expectedErr bool
}{
"empty SQL statement": {
sql: "",
Copy link
Collaborator

Choose a reason for hiding this comment

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

could fill in the expectedOps and expectedErr fields here too.

@kvch kvch enabled auto-merge (squash) February 19, 2025 16:29
@kvch kvch merged commit bc24f9e into xataio:main Feb 19, 2025
27 checks passed
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.

2 participants