Skip to content

Conversation

@kravcneger
Copy link
Contributor

Guys, at least one type of constraint can be placed after the table definition, just like indexes. It also improves the readability of the schema.


if options[:indexes_after_tables] == true
# Extract indexes, remove comments and place them just after the respective tables
if options[:meta_tables_after_main] == true
Copy link
Collaborator

Choose a reason for hiding this comment

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

For backwards comparability it would be better to check both the new and old setting:

if options[:indexes_after_tables] || options[:meta_tables_after_main]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kravcneger kravcneger force-pushed the feature-deferrable-constraints-after-main branch 2 times, most recently from 041591a to e5d7621 Compare June 11, 2025 14:19
@kravcneger kravcneger requested a review from seanlinsley June 20, 2025 17:43

if options[:indexes_after_tables] == true
# Extract indexes, remove comments and place them just after the respective tables
if options[:indexes_after_tables] || options[:meta_tables_after_main]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you want to leave a comment to indicate options[:indexes_after_tables] is the old option and is deprecated, and maybe specify a version where it will be removed? We could also add the deprecation announcement into the release notes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@andyatkinson like so?

@kravcneger kravcneger requested a review from andyatkinson July 9, 2025 11:34
@lfittl
Copy link
Owner

lfittl commented Nov 26, 2025

@kravcneger Thanks for your patience on this pull request!

I just discussed this with @seanlinsley and colleagues, and we had one more change we'd like to make here before merging: The term meta_tables_after_main feels a bit confusing, how about we instead call the setting group_table_definition?

If you agree with that, happy to have you make the change, or we can also edit the PR and then approve and merge if you prefer.

@kravcneger kravcneger force-pushed the feature-deferrable-constraints-after-main branch 2 times, most recently from 78ae86b to b1e85c3 Compare December 1, 2025 17:39
@kravcneger kravcneger force-pushed the feature-deferrable-constraints-after-main branch from b1e85c3 to b77ea5a Compare December 1, 2025 18:02
@lfittl lfittl merged commit d05d9ac into lfittl:master Dec 1, 2025
2 checks passed
@lfittl
Copy link
Owner

lfittl commented Dec 1, 2025

@kravcneger Thanks for updating this, and appreciate your patience in getting this merged!

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.

4 participants