-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 a more "advanced" example #1428
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Reviewed up to main.rs)
chrono = "0.4.0" | ||
diesel = { version = "1.0.0-beta1", features = ["postgres", "chrono"] } | ||
dotenv = "0.10.0" | ||
tempfile = "2.2.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now this is weird: I just realized all the dependencies were sorted alphabetically, except for this line. And it's my fault 😅
@@ -0,0 +1,2 @@ | |||
DROP FUNCTION IF EXISTS diesel_manage_updated_at(_tbl regclass); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Random remark: This file should probably have a -- This file was automatically created by Diesel
header
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Open an issue? ;)
if_not_present(dotenv::var("BLOG_PASSWORD"), NoPasswordSet) | ||
} | ||
|
||
fn if_not_present<T>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a weird name… I'd probably write an fn get_dotenv_or_err_with(&str, AuthenticationError)
that also includes the dotenv::var
. Making this a very generic function doesn't really help here, as it internally depends on dotenv::Error
anyway.
use schema::users; | ||
|
||
#[derive(Debug)] | ||
pub enum AuthenticationError { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd move this (and the From impl) just above the helper functions (that start with find_user
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice, the comments here conclude my review.
Do the other examples have README.md
files? Maybe we can add one here. It doesn't really map 1:1 to a guide (yet), and pointing out a few interesting aspects couldn't hurt.
use diesel::pg::Pg; | ||
use diesel::types::BigInt; | ||
|
||
pub trait Paginate: Sized { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not that it is particularly useful here, but in the 'leading by example' sense we should probably add some doc comments to the public items in this module. At least, we should add a module level doc comment that links to the #1437 guide (once it is online)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. I'm going to merge without doing this so I can publish a guide referencing code here, but I will make sure to come back and do this after we ship 1.0
pub status: Status, | ||
} | ||
|
||
pub enum Status { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd call more attention to this enum here and its Queryable impl as it's not just boilerplate but actually one of the 'advanced' features :) So maybe add a doc comment that describes how this is used to map NULL
to Draft
, and how it's a good idea to do this instead of just using Option<NaiveDateTime>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Advanced querying guide will
@@ -0,0 +1,38 @@ | |||
table! { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add
// generated by `diesel print-schema`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But then I can't do diesel print-schema > src/schema.rs
This app is eventually going to be used as part of the advanced querying guide. Unlike the getting started guide, that guide will not be a step by step "building this app" style guide, but will instead refer to some of the code in this example for its code examples, as I want to have them presented with broader context if the reader wants it. Since this example is meant to be separate from the guide, I figured it made sense to submit it on its own. Originally I chose a CLI blog for the getting started guide because I wanted the example purely focused on Diesel, with little to no noise from CLI stuff or a web framework. Clearly that is not the case here. In fact, a web app probably would be a better example. However, I'm so amused by the idea of a "CLI blog" that I decided to double down on it and make a multi-tenant blog wiht commenting functionality and pagination. Of course this is a pointless exercise, since having auth logic makes no sense when the client requires a direct database connection... Anyway, this example is meant to provide some examples for the following more complex use cases: - Various types of joins and associations. Demonstrating data with multiple associations that go to the same table multiple times, and is difficult to describe unambiguously - When to use joins vs `belonging_to` - Nesting queryables (see the structs used in auth) - Dealing with structs that don't contain all fields from a table - Demonstrating that `Queryable` represents the results of a query, not a single table - I want to restructure how `all_posts` works. I really want to have a struct that is used for "comment with username" and "comment with post title" but that doesn't work well today. I'm going to revisit for 1.1 - Real world use of `returning` - Real world constraint error handling (I may change this to be `.on_conflict(username).do_nothing()` and handle it there so this maps more easily to SQLite and MySQL. If we're being pedantic I should be checking the constraint name in that match arm) - Showing how to deal with "sometimes update this field" without using `AsChangeset`. - Showing that various query builder methods compose together - Generally just having more examples of non-trivial queries.
Structopt allows generating clap-based CLI arg parsers by using derives on structs/enums. Crucially, it also handles converting arguments to their expected types. My goal here was to reduce the CLI parsing boilerplate (this code lives in Diesel's example directory and should thus focus on Diesel) while also switching to a style that will most likely be used by the next version of clap itself. There still is more boilerplate[^1] than I'd like to have, but at least it's now a well-typed enum, and we can skip all the type conversion steps. Also note that the line count is slightly skewed by my additional help texts. [^1]: For example: If there was a "convert enum variant name to kebab-case[^2] option I'd love to use it instead of all these `name = "foo_bar"` attributes. [^2]: Yes we should totally use kebab-case and not snake_case here.
e173651
to
59a4fee
Compare
This app is eventually going to be used as part of the advanced querying
guide. Unlike the getting started guide, that guide will not be a step
by step "building this app" style guide, but will instead refer to some
of the code in this example for its code examples, as I want to have
them presented with broader context if the reader wants it.
Since this example is meant to be separate from the guide, I figured it
made sense to submit it on its own.
Originally I chose a CLI blog for the getting started guide because I
wanted the example purely focused on Diesel, with little to no noise
from CLI stuff or a web framework. Clearly that is not the case here. In
fact, a web app probably would be a better example. However, I'm so
amused by the idea of a "CLI blog" that I decided to double down on it
and make a multi-tenant blog wiht commenting functionality and
pagination. Of course this is a pointless exercise, since having auth
logic makes no sense when the client requires a direct database
connection...
Anyway, this example is meant to provide some examples for the following
more complex use cases:
multiple associations that go to the same table multiple times, and is
difficult to describe unambiguously
belonging_to
Queryable
represents the results of a query, nota single table
all_posts
works. I really want to havea struct that is used for "comment with username" and "comment
with post title" but that doesn't work well today. I'm going to
revisit for 1.1
returning
.on_conflict(username).do_nothing()
and handle it there so this mapsmore easily to SQLite and MySQL. If we're being pedantic I should be
checking the constraint name in that match arm)
AsChangeset
.