jackc / tern

The SQL Fan's Migrator
MIT License
925 stars 68 forks source link

add Filename to MigrationPgError #51

Closed davidmdm closed 2 years ago

davidmdm commented 2 years ago

Simply put, when using tern/migrate to run migrations, if a migration fails the error is returned and the PgError message is printed to the user. It would be useful to show to the user which of their migration files caused the error.

Intended use:

if err := m.Migrate(ctx); err != nil {
  var migrationErr migrate.MigrationPgError
  if errors.As(err, &migrationErr) {
    fmt.Fprintf(os.Stderr, "failed to perform migration(s): %s: %v\n", migrationErr.Filename, migrationErr.PgError)
    os.Exit(1)  
  }
  // handle error
}
davidmdm commented 2 years ago

@jackc

I didn't want to change anything or cause any tests to break, so I added the Filename property only, so that it could be used by programs that use tern/migrate.

Would you be open to changing the MigrationPgError Error to something like this?

type MigrationPgError struct {
    Filename string
    Sql      string
    *pgconn.PgError
}

func (e MigrationPgError) Error() string {
    if e.Filename == "" {
        return e.PgError.Error()
    }
    return fmt.Sprintf("%s: %s", e.Filename, e.PgError.Error())
}
jackc commented 2 years ago

While migrations typically do come from files it isn't strictly required, so I would suggest that instead of Filename the field should be named Name or MigrationName.

As far as changing MigrationPgError.Error(), I think that is okay as long we can keep the existing error output for the CLI tool.

davidmdm commented 2 years ago

@jackc

Here is what I did in this last commit:

jackc commented 2 years ago

LGTM

davidmdm commented 2 years ago

@jackc

Can you tag master with this new commit? I want to use this new error context in my tern scripts.

Much appreciated :)

jackc commented 2 years ago

Done.