-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Description
Setup
Versions
- Rust: rustc 1.31.0-nightly (e7f5d4805 2018-10-18)
- Diesel: 1.3.3
- Database: Postgresql
- Operating System: macOS
Feature Flags
- diesel: postgres, r2d2
Problem Description
I just started working with Diesel as part of a Rocket server app I'm building, and I'm working my way through Rocket's guide to get up to speed on the ins and outs of working with Rocket. I've gotten to the point where I need to set up my database, so I jumped from Rocket's section on state with databases (using Diesel) to Diesel's guide and started following along.
In the past I've used ORMs like Sequelize, which automatically adds columns for created_at and updated_at to generated models, and I know I want to include these fields in my models in Diesel. However, I ran into some confusion regarding the timestamp helpers while following the guide, and while I think I understand how all the related parts fit together I think the explanations could be smoothed out a bit for users new to Diesel.
First, the guide mentions:
diesel setupThis will create our database (if it didn't already exist), and create an empty migrations directory that we can use to manage our schema (more on that later).
As far as I can tell, this hasn't been true since #991 landed; the migrations/ directory is not empty, and the generated migrations/00000000000000_diesel_initial_setup caught me off guard because the guide said the migrations/ folder should be empty.
I think it would also be really helpful to see these files be written in a similar way to how the path of user-generated migrations are output to the console when migrations are created.
Instead of:
$ diesel setup
Creating migrations directory at: /absolute/path/to/project/migrationsI think it would be helpful to see:
$ diesel setup
Creating migrations directory at: /absolute/path/to/project/migrations
Creating migrations/00000000000000_diesel_initial_setup/up.sql
Creating migrations/00000000000000_diesel_initial_setup/down.sqlNext, I think the explanation comment inside up.sql could be tweaked to better clarify the provided function.
-- Sets up a trigger for the given table to automatically set a column called
--updated_atwhenever the row is modified (unlessupdated_atwas included
-- in the modified columns)
I think "to automatically set a column" should actually read "to automatically update a column" or "to automatically update the value of a column"; if I understand correctly the timestamp in the updated_at column gets updated to the current time as one would expect if Diesel is automatically managing this column.
It also wasn't immediately clear to me, but am I correct that the example in the explanation comment is SQL for an example migration to create a Users table, and as such would be created in a different file? My initial impression was that code that followed the pattern of the example would be added to migrations/00000000000000_diesel_initial_setup/up.sql, but I don't think that initial impression is correct.
If the example would in fact live in a different file, I think it would be very helpful to annotate the SQL snipped with an example file name, such as Filename: migrations/XXXXXXXXXXXXX_create_users.sql just as the Rust book does for multiple files in, for example, the section on mod and the file system. I think this would help clarify for those new to Diesel that code like the explanation comment's example should actually live in a separate file.
I would be very happy to put up a PR (or more) to address the above, and I would really appreciate any feedback you might have about these suggestions. As I only just started working with Diesel today, there may have been something I missed in the guide or other docs about this feature, but I'd still like to see if we can improve this feature and make it a bit more visible.
Checklist
- I have already looked over the issue tracker for similar issues.
- This issue can be reproduced on Rust's stable channel. (Your issue will be
closed if this is not the case)