Skip to content

Allow 'Down' migrations in .sql files#530

Merged
dolezel merged 7 commits into
salsita:masterfrom
thejetou:sql-down
Dec 11, 2019
Merged

Allow 'Down' migrations in .sql files#530
dolezel merged 7 commits into
salsita:masterfrom
thejetou:sql-down

Conversation

@thejetou

@thejetou thejetou commented Dec 7, 2019

Copy link
Copy Markdown
Contributor

This commit allows SQL files to have down migrations via comments.

Anything below -- Up migration will be treated as up, anything
below a -- Down migration will be treated as down.

A migration file can have either or both.

This commit allows SQL files to have down migrations via comments.

Anything below `-- Up migration` will be treated as `up`, anything
below a `-- Down migration` will be treated as `down`.

A migration file can have either or both.
Comment thread templates/migration-template.sql Outdated
Comment thread src/runner.ts Outdated
Comment thread src/runner.ts Outdated
@dolezel

dolezel commented Dec 11, 2019

Copy link
Copy Markdown
Contributor

I would do three things:

  1. write tests
  2. do not explicitly require order of comments
  3. if down migration is not present, trying to run it should fail

@dolezel
dolezel merged commit 3c4ec54 into salsita:master Dec 11, 2019
@Shinigami92

Copy link
Copy Markdown
Collaborator
  1. write tests

👍


  1. do not explicitly require order of comments

👍


  1. if down migration is not present, trying to run it should fail

What do you mean?

-- up migration
insert into ...

-- down migration
-- nothing to do

Do you think that should fail? Why?

@dolezel

dolezel commented Dec 11, 2019

Copy link
Copy Markdown
Contributor

It should keep current behavior. Where you can't run down migrations on SQL files.
The same as for normal migrations. You can write explicit down migrations, disabled it, or Terry to infer it from up migration. But you can't infer down migration from SQL string. It failed down migrations

@Shinigami92

Copy link
Copy Markdown
Collaborator

I'm still a little bit confused (and my english skills lags 😅)

I totally understand that we can't infer down migrations from up migrations, this wasn't my point
But I dont think it should fail when there is no SQL found below the down migration-comment

@dolezel

dolezel commented Dec 11, 2019

Copy link
Copy Markdown
Contributor

I'm probably describing it wrong. It should fail if running down migrations and there is no down migration comment.

@Shinigami92

Shinigami92 commented Dec 11, 2019

Copy link
Copy Markdown
Collaborator

OK

I dont think so

-- up migration
insert into ...

should work, imo

edit:
so IMO i mean follwoing should all work

example 1

-- up migration
insert into ...

same as example 1

insert into ...

example 2

-- down migration
delete from ...

example 3

-- down migration
delete from ...

-- up migration
insert into ...

example 4

-- down migration
delete from ...

-- up migration

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.

3 participants