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

Add/fix fields in CREATE MIGRATION #4550

Merged
merged 4 commits into from
Oct 21, 2022
Merged

Add/fix fields in CREATE MIGRATION #4550

merged 4 commits into from
Oct 21, 2022

Conversation

tailhook
Copy link
Contributor

@tailhook tailhook commented Oct 19, 2022

  1. message field is now reflected in schema::Migration
  2. Added generated_by field to the migration
  3. DDL-generated migrations are now marked with generated_by := MigrationGeneratedBy.DDLStatement.

@tailhook
Copy link
Contributor Author

Also I've noted that you can't do SET message := in START MIGRATION:

edgedb> start migration to { module default { type X; }};
OK: START MIGRATION
edgedb[tx]> populate migration;
OK: POPULATE MIGRATION
edgedb[tx]> set message := 'from started';
error: EdgeQLSyntaxError: Unexpected 'message'
  ┌─ query:1:5
  │
1 │ set message := 'from started';
  │     ^^^^^^^ error

edgedb[tx:failed]> rollback;

This is a bug, right? But I'm not sure if it's fixable, as it conflicts with set {module|global|alias} syntax a bit.

1. `message` field is now reflected in `schema::Migration`
2. Added `generated_by` field to the migration
@msullivan
Copy link
Member

This is a bug, right? But I'm not sure if it's fixable, as it conflicts with set {module|global|alias} syntax a bit.

It's probably fixable, in this specific case of set message; probably not for arbitrary things?

Copy link
Member

@msullivan msullivan left a comment

Choose a reason for hiding this comment

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

This is all fairly reasonable but I think that there is a simpler way that leverages our generic mechanisms for handling SET. Try to use that and follow up with me if it doesn't work?

self.val = qlast.CreateMigration(
name=kids[2].val.name,
parent=kids[2].val.parent,
body=kids[3].val.body,
message=message,
Copy link
Member

Choose a reason for hiding this comment

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

I think that if we populate the commands field here with kids[3].val.fields, the generic mechanisms for handling SET field := ... should pick up the commands and work without needing to have a message or generated_by field in qlast.CreateMigration, without needing to pick them out in the parser, and thus also without needing to handle them in _cmd_from_ast

@tailhook
Copy link
Contributor Author

This is all fairly reasonable but I think that there is a simpler way that leverages our generic mechanisms for handling SET. Try to use that and follow up with me if it doesn't work?

It works. Thanks! Feels a bit too magical for someone not familiar enough with the codebase, though.

@tailhook
Copy link
Contributor Author

Oh, by the way, I've just noticed that I've added uniqueness check into the quite generic check. Please double check that it's fine (looks like it's only used in two places, but I'm not sure). Maybe some SET field statements should be overridable?

@msullivan
Copy link
Member

Oh, by the way, I've just noticed that I've added uniqueness check into the quite generic check. Please double check that it's fine (looks like it's only used in two places, but I'm not sure). Maybe some SET field statements should be overridable?

Yeah, that seems right.

Copy link
Member

@msullivan msullivan left a comment

Choose a reason for hiding this comment

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

Oh I thought I approved this yesterday!

@msullivan msullivan merged commit 748d873 into master Oct 21, 2022
@msullivan msullivan deleted the migration_fields branch October 21, 2022 17:37
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