Adding `barrel` feature #1573

spacekookie · 2018-02-22T15:24:16Z

This is the initial version of integrating barrel into diesel. A few things are still not optimal and I'd like to get some feedback regarding that.

There are some barrel specific functions and structs in the migrations_internal module which are only compiled when using barrel. I would love to keep those parts in the barrel crate but can't because of cyclic dependencies. Is there a different way to solve this? If so, how?
Currently (for testing) the barrel feature is always enabled on diesel. How would I add features = [ "barrel"] to the migration_internals crate, depending on the external flag? Is there a more elegant way of doing this?
Should the code added to diesel_cli be conditionally added with a barrel feature flag too?

Additionally the barrel dependency currently points to git which should be changed once barrel 0.2 is released

sgrif · 2018-02-22T15:29:20Z

diesel_cli/src/cli.rs

+                        .long("type")
+                        .short("t")
+                        .takes_value(true)
+                        .help("Specify the migration type."),


Can we add .possible_values(&["sql", "barrel"]) and .default_value("sql")?

sgrif · 2018-02-22T15:29:41Z

diesel_cli/src/cli.rs

@@ -51,6 +51,13 @@ pub fn build_cli() -> App<'static, 'static> {
                             for most use cases.",
                        )
                        .takes_value(true),
+                )
+                .arg(
+                    Arg::with_name("type")


Arg names should be uppercase by convention.

sgrif · 2018-02-22T15:30:22Z

diesel_cli/src/main.rs

-            down.write_all(b"-- This file should undo anything in `up.sql`")
-                .unwrap();
+            match migration_type {
+                Some("barrel") => {


This needs to be #[cfg]d.

sgrif · 2018-02-22T15:30:34Z

diesel_cli/src/main.rs

+                    barrel_migr.write(b"/// Handle down migrations \n").unwrap();
+                    barrel_migr
+                        .write(b"fn down(migr: &mut Migration) {} \n")
+                        .unwrap();


All of this code should live in Barrel, not Diesel.

sgrif · 2018-02-22T15:30:50Z

diesel_cli/src/main.rs

+                        .unwrap();
+                }
+                _ => {
+                    let up_path = migration_dir.join("up.sql");


Can we pull this out into a function?

sgrif · 2018-02-22T15:31:54Z

diesel_migrations/migrations_internals/src/migration.rs

@@ -49,6 +58,39 @@ pub fn migration_from(path: PathBuf) -> Result<Box<Migration>, MigrationError> {
    }
 }

+#[cfg(feature = "barrel")]
+struct BarrelMigration(PathBuf, String, String, String);


Can we just move the migration trait from diesel_migrations into diesel so this code can live in Barrel?

sgrif

This is a good start, but there's more restructuring to be done. Given that we don't maintain Barrel, I want to have as little Barrel specific code in Diesel as possible.

sgrif · 2018-02-23T23:30:20Z

CI is failing.

spacekookie · 2018-02-24T00:19:28Z

Yea, there is a problem with the code I don't know how to solve. I messaged you on gitter about it.

Also, looking at what's failing, it seems to be fine on one of the nightly branches. barrel does currently require nightly until rust#44490 is stabilised.

sgrif · 2018-02-24T02:03:40Z

Oh whoops! Sorry!

…

On Fri, Feb 23, 2018, 5:19 PM Katharina ***@***.***> wrote: Yea, there is a problem with the code I don't know how to solve. I messaged you on gitter about it — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1573 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABdWK9hPJnh0r9DzpXAhM84gbtuHQMWWks5tX1WSgaJpZM4SPfcY> .

sgrif · 2018-02-27T21:17:41Z

diesel/src/migration/mod.rs

+/// A migration name
+#[allow(missing_debug_implementations)]
+#[derive(Clone, Copy)]
+pub struct MigrationName<'a> {


Why did this need to move?

sgrif · 2018-02-27T21:18:10Z

diesel/src/migration/mod.rs

@@ -19,3 +20,67 @@ pub trait Migration {
        None
    }
 }
+
+/// A migration name


Can we document what this actually does rather than just repeating the name of the type?

sgrif · 2018-02-27T21:18:20Z

diesel/src/migration/mod.rs

+#[allow(missing_debug_implementations)]
+#[derive(Clone, Copy)]
+pub struct MigrationName<'a> {
+    /// Wraps around a migration


Can we provide more information than just repeating the type?

sgrif · 2018-02-27T21:18:50Z

diesel/src/migration/mod.rs

+    pub migration: &'a Migration,
+}
+
+/// Get the name of a migration


Can we provide more information than just repeating the function/argument names? Maybe at least a usage example?

spacekookie · 2018-02-27T21:45:19Z

That's a fair point, I moved the MigrationName stuff back to migrations_internals. I don't really know what else to document about it. Considering that nothing else in that module is documented I think this should be alright.

spacekookie · 2018-04-17T00:22:17Z

Ping @sgrif it builds now! 🎉 😄

sgrif · 2018-04-17T11:08:27Z

I'm at a conference, but one of the other committers can take care of merging. I'd like to see this squashed down.

…

On Mon, Apr 16, 2018 at 8:22 PM Katharina ***@***.***> wrote: Ping @sgrif <https://github.com/sgrif> it builds now! 🎉 😄 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1573 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABdWKwQjRoJoyX2lLe8NTqaS5hQonqyCks5tpTW6gaJpZM4SPfcY> .

spacekookie added 9 commits February 15, 2018 14:05

Optionally adding barrel crate to diesel_migrations

7f841e5

Initial support for barrel migrations in diesel cli

5739101

Using the barrel diesel feature

f258e56

Changing barrel call signature to build Migration on diesel side

439620e

Connecting barrel to diesel migration layouts

4cd7b79

Formatting with rustfmt

25267f9

Changing generated migration function signatures

9b95920

Cleaning up migration process slightly

b4bacf4

Pointing barrel to github version

67fd97e

sgrif reviewed Feb 22, 2018

View reviewed changes

diesel_cli/src/main.rs Outdated

.unwrap();

}

_ => {

let up_path = migration_dir.join("up.sql");

Copy link

Member

sgrif Feb 22, 2018

Choose a reason for hiding this comment

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

Can we pull this out into a function?

sgrif reviewed Feb 22, 2018

View reviewed changes

sgrif requested changes Feb 22, 2018

View reviewed changes

spacekookie added 3 commits February 22, 2018 17:24

Implementing requested changes in diesel_cli

4f64f21

Fix git repository link

e46570d

Preliminarily commiting refactoring changes

7f4b65b

Moving Migration trait to core diesel library

487404d

sgrif reviewed Feb 27, 2018

View reviewed changes

spacekookie added 2 commits February 27, 2018 22:20

Adjusting the module comment formatting to let CI pass

a4cb6dd

Moving MigrationName back to migrations_internals

f0b153f

spacekookie and others added 27 commits April 16, 2018 20:01

Implementing requested changes in diesel_cli

3101a99

Preliminarily commiting refactoring changes

d5330d2

Moving Migration trait to core diesel library

a7cd615

Adjusting the module comment formatting to let CI pass

9dd24cf

Moving MigrationName back to migrations_internals

8960d05

Adding new import to fix some Migration tests

cfc453d

Changing the migration generation function signature

2fc3637

Another clippy change

6b902d4

Renaming external feature flag. Changing barrel version to non-git

051661f

Changing barrel version to non-git in diesel_cli

b4a2b3b

Adjust the diesel feature flag on barrel

5029b64

Re-exporting diesel::migration::* to avoid a breaking change

4eebacf

Cleaning up diesel_migration exports

03e0248

Adjusting a few function calls into barrel to the v0.2.0 API

b7bd404

Re-adding more re-exports to diesel_migrations

3bb1233

Changing feature and type flag names in diesel_cli

a5d375d

Making error enums non exhaustive. Only exporting speific error types

e8bd6f5

Breaking formatting

6693e87

Fixing formatting again

13e8caa

Touching code to make github see changes

0dc23b8

Changing formatting back to the way it's meant to be

8595def

Handling __NonExaustive options for migration errors

f84f394

Not exposing the entire mirations/error module anymore

c1a217f

Fixing some formatting issues

5168672

Cleaning a bad merge artefact

2086b99

Resolving rebase differences

ed20737

Fixing a bad rebase

ef98ddc

killercup merged commit fb1e61b into diesel-rs:master Apr 17, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding `barrel` feature #1573

Adding `barrel` feature #1573

spacekookie commented Feb 22, 2018

sgrif Feb 22, 2018

sgrif Feb 22, 2018

sgrif Feb 22, 2018

sgrif Feb 22, 2018

sgrif Feb 22, 2018

sgrif Feb 22, 2018

sgrif left a comment

sgrif commented Feb 23, 2018

spacekookie commented Feb 24, 2018 •

edited

Loading

sgrif commented Feb 24, 2018 via email

sgrif Feb 27, 2018

sgrif Feb 27, 2018

sgrif Feb 27, 2018

sgrif Feb 27, 2018

spacekookie commented Feb 27, 2018

spacekookie commented Apr 17, 2018

sgrif commented Apr 17, 2018 via email

Adding barrel feature #1573

Adding barrel feature #1573

Conversation

spacekookie commented Feb 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sgrif left a comment

Choose a reason for hiding this comment

sgrif commented Feb 23, 2018

spacekookie commented Feb 24, 2018 • edited Loading

sgrif commented Feb 24, 2018 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spacekookie commented Feb 27, 2018

spacekookie commented Apr 17, 2018

sgrif commented Apr 17, 2018 via email

Adding `barrel` feature #1573

Adding `barrel` feature #1573

spacekookie commented Feb 24, 2018 •

edited

Loading