Skip to content
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

Diesel example #126

Closed
wants to merge 14 commits into from
Closed

Diesel example #126

wants to merge 14 commits into from

Conversation

n-pochet
Copy link
Contributor

@n-pochet n-pochet commented Feb 1, 2018

This PR addresses the issue #77
It is the first draft and could certainly be improved.
Thank you beforehand for your advice and remarks!

Copy link
Contributor

@bradleybeddoes bradleybeddoes left a comment

Choose a reason for hiding this comment

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

Overall this looks like a really great start, very happy to keep working with you on this. 👍

It may be that we hold this example for a short while before merging (once we've sorted out the few identified issues) as the Diesel middleware still needs some rough edges sanded off itself, namely proper async consideration, which will necessitate further change here I am guessing.

Cargo.toml Outdated
@@ -11,4 +11,5 @@ members = [
"examples/hello_world",
"examples/hello_router",
"examples/basic_router",
"examples/basic_diesel",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest we call this example simply "diesel"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed. But I still need to name my package in Cargo.toml basic_diesel or I will get a conflict with the real diesel crate.

@@ -0,0 +1 @@
DATABASE_URL=.posts.db
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest we hardcode this for the example, goal being to streamline 3rd party overheads/concepts as much as possible.

One of the examples I have planned is going to give some ideas for external configuration loading so we'll cover this then.

@@ -0,0 +1,2 @@
-- This file should undo anything in `up.sql`
Copy link
Contributor

Choose a reason for hiding this comment

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

suggest remove comment

@@ -0,0 +1,7 @@
-- Your SQL goes here
Copy link
Contributor

Choose a reason for hiding this comment

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

suggest removing comment



// The URL of the database.
static DATABASE_URL: &'static str = "posts.db";
Copy link
Contributor

Choose a reason for hiding this comment

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

I note the database was committed, suggest removing the binary, it won't be necessary for folks.

}

/// Create the table if needed.
fn create_table(conn: &SqliteConnection) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@n-pochet
Copy link
Contributor Author

n-pochet commented Feb 5, 2018

@bradleybeddoes Thanks for the review. I'll make the changes taking your advice into account.

@bradleybeddoes
Copy link
Contributor

I'm just thinking out loud here and feel free to not like this idea, but following my comment on #108 I started considering this PR again.

I was wondering what you thought of also giving this example the 'web store' treatment. Essentially changing it from creating and listing Post to creating and listing Widget as part of the web store "api" that I started to hint at here: https://github.com/gotham-rs/gotham/blob/master/examples/basic_router/src/main.rs#L62

@n-pochet
Copy link
Contributor Author

n-pochet commented Feb 6, 2018

Sure. It makes more sense to have a "unified" set of examples. I'll change that.
Do you have any idea of what the definition of a Widget should be?

@bradleybeddoes
Copy link
Contributor

Oops! I guess this could be very easily confused by folks who haven't seen that before!!.

Perhaps a more universal name here would be Product/products instead of Widget/widgets.

Maybe the products we "sell" could be some of the Rust language merchandise I've seen floating about such as stickers, shirts and the like. e.g:

I'm open to suggestions!

@n-pochet n-pochet force-pushed the diesel-example branch 2 times, most recently from 8c88520 to 4a1e9da Compare February 9, 2018 12:51
@dsvensson dsvensson mentioned this pull request Mar 7, 2018
@nyarly
Copy link
Contributor

nyarly commented Sep 29, 2018

@n-pochet It's been some time since this PR saw attention (from us!) How do you feel about it these days? Should it get a new review pass?

Make it possible to insert `NewPost` by calling the POST method and
passing a `NewPost` object in JSON
Rename the example folder from `basic_diesel` to `diesel`
Use `embed_migrations!()` to simplify the logic in `lib.rs`
Adapt `README.md`
Change the migrations scripts
Change all the references (functions and structs)
@n-pochet
Copy link
Contributor Author

n-pochet commented Nov 8, 2018

Hi @nyarly,

Sorry for the late reply. I have a ton of other projects running.
I rebased and fixed the issues in the example. Feel free to review.

* Change versions in `Cargo.toml`
* Use the new versions of the functions
@bradleybeddoes
Copy link
Contributor

Closing due to lack of input, please re-open if still a wanted PR.

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.

None yet

3 participants