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 support for custom migration_dir to disel migration revert and redo #868

Merged
merged 1 commit into from May 11, 2017

Conversation

Projects
None yet
3 participants
@Eijebong
Copy link
Member

Eijebong commented Apr 20, 2017

This fixes a part of #857
The rest will follow once this one has been merged :)

@Eijebong Eijebong force-pushed the Eijebong:fix_migration_directory branch 2 times, most recently from 22ff7e9 to 43ee077 Apr 21, 2017

@killercup
Copy link
Member

killercup left a comment

LGTM.

Seeing as the first line in this diff already introduces a change to our public API (though a tiny one that doesn't trickle down to any other methods AFAIKT), it might be prudent to think about refactoring the migration API.

Currently, diesel::migrations consists of a bunch of static functions, of which most have some sort of argument to supply the source for migrations. Intuitively, I'd like to group them as a struct. How about this:

type Migration = PathBuf;

struct Migrations {...} // <- plural

impl Migrations {
  /// Assumes default directory
  fn new() -> Result<Self>;

  // The change introduced in this PR
  fn with_directory<P: AsRef<Path>>(path: P) -> Result<Self>;

  /// Return found migrations
  fn all(&self) -> Result<Vec<Migration>>;

  /// List of migrations and whether they have been applied
  fn diff(&self) -> Result<Vec<(Migration, bool)>>;

  /// Run pending migrations
  fn run() -> Result<()>;
}
@@ -132,38 +132,37 @@ pub fn mark_migrations_in_directory<Conn>(conn: &Conn, migrations_dir: &Path)
///
/// See the [module level documentation](index.html) for information on how migrations should be
/// structured, and where Diesel will look for them by default.
pub fn revert_latest_migration<Conn>(conn: &Conn) -> Result<String, RunMigrationsError> where
pub fn revert_latest_migration<Conn>(conn: &Conn, migrations_dir: &Path) -> Result<String, RunMigrationsError> where

This comment has been minimized.

@killercup

killercup Apr 21, 2017

Member

This is a breaking change.

This comment has been minimized.

@Eijebong

Eijebong Apr 21, 2017

Member

Erf, this is public API ? :(

This comment has been minimized.

@killercup

killercup Apr 21, 2017

Member

FTR, my "is this public API?" test: copy fn name, go to http://docs.diesel.rs, search for fn name, #results > 0 == public api

This comment has been minimized.

@sgrif

sgrif Apr 30, 2017

Member

This is the correct answer. And while breaking changes are allowed since we're still pre-1.0, I would like to keep this function as is. Most of these functions have the do_thing and do_thing_in_directory

This comment has been minimized.

@Eijebong

Eijebong Apr 30, 2017

Member

Okay' I'll fix that :)

@sgrif
Copy link
Member

sgrif left a comment

This looks fine overall, but I'd prefer to introduce a new function instead of changing the API of revert_latest_migration.

@Eijebong Eijebong force-pushed the Eijebong:fix_migration_directory branch 2 times, most recently from 3573e3b to 9db8fd0 May 3, 2017

@Eijebong

This comment has been minimized.

Copy link
Member

Eijebong commented May 7, 2017

Is there still something blocking this one ?

.map(|_| latest_migration_version)

This comment has been minimized.

@sgrif

sgrif May 9, 2017

Member

✂️

@@ -219,15 +221,15 @@ fn search_for_cargo_toml_directory(path: &Path) -> DatabaseResult<PathBuf> {

/// Reverts the most recent migration, and then runs it again, all in a
/// transaction. If either part fails, the transaction is not committed.
fn redo_latest_migration<Conn>(conn: &Conn) where
fn redo_latest_migration_<Conn>(conn: &Conn, migrations_dir: &Path) where

This comment has been minimized.

@sgrif

sgrif May 9, 2017

Member

Why the function name change?

This comment has been minimized.

@Eijebong

Eijebong May 9, 2017

Member

o_o how did this even get in there ?

@sgrif

This comment has been minimized.

Copy link
Member

sgrif commented May 9, 2017

LGTM modulo style nits

@Eijebong Eijebong force-pushed the Eijebong:fix_migration_directory branch from 9db8fd0 to 045ce48 May 9, 2017

@Eijebong Eijebong force-pushed the Eijebong:fix_migration_directory branch from 045ce48 to b1ce45b May 11, 2017

@Eijebong Eijebong merged commit a77622e into diesel-rs:master May 11, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Eijebong added a commit to Eijebong/diesel that referenced this pull request May 12, 2017

Eijebong added a commit to Eijebong/diesel that referenced this pull request May 12, 2017

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