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

RunMigrationsError provides no information about the specific migration that failed #1605

Open
Aaron1011 opened this Issue Mar 28, 2018 · 3 comments

Comments

Projects
None yet
3 participants
@Aaron1011

Aaron1011 commented Mar 28, 2018

While RunMigrationsError provides information about the type of migration error that occurred, it provides no information (e.g. the file path) about the specific migration that caused the error.

Since the RunMigrationsError enum has neither a non_exhaustive attribute nor a #[doc(hidden)] variant, adding new variants is a breaking change. It was suggested on Gitter that a new error type (and corresponding functions) be created, and that the current RunMigrationsError and run_migrations be deprecated.

@sgrif

This comment has been minimized.

Member

sgrif commented Mar 28, 2018

Copying my comments from the gitter convo (and pinging the power rangers):

I can say with certainty that I intended to make that enum (and MigrationError) extensible, and it slipped through the cracks
(I wouldn't turn it into a struct, but that's semantics)
@diesel-rs/core I would like to get the rest of the core team's opinion here. I don't think there's anything we can do without a semver major version bump. I'm opposed to separating the version of diesel_migrations from the rest of the framework, and I'm also opposed to a semver-major bump on the whole shebang. That said, this enum should have been made extensible from the start. What do y'all think?

@Diggsey

This comment has been minimized.

Contributor

Diggsey commented Mar 28, 2018

I think a couple of improvements could be made in the process: the names of some of the functions are a little odd (for example, the function to find all the migrations is called mark_migrations_in_directory) and a lot of the design seems to be quite heavily based on the implementation (for example, name() not being a method on the Migration trait, but a standalone function).

This would be a good opportunity to refactor some of this: for example creating a MigrationSet trait instead of passing around a raw PathBuf, which has methods to list Migrations.

@sgrif

This comment has been minimized.

Member

sgrif commented Mar 28, 2018

Yeah, a redesign was originally planned for 1.0 but we agreed that we could \ deprecate the current API backwards compatibly. That does not apply to types

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment