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

Minor edits to the AR guide #51333

Merged
merged 1 commit into from Mar 15, 2024
Merged

Minor edits to the AR guide #51333

merged 1 commit into from Mar 15, 2024

Conversation

fxn
Copy link
Member

@fxn fxn commented Mar 15, 2024

Hey @bhumi1102, awesome job in #51226!

I have made a pass over the guide to polish a few totally minor things. Let me explain some of them:

  • Even if the following sections may expand on what the text introduces, at that level you don't normally end a section with a colon. Saying "below" already does the job, and a period is more conventional.
  • In the list with BookClub and book_clubs, there's this pattern where you have a list item, a hyphen, and a sentence, that is found in other places in this guide. In the rest of them, the first letter of the sentence is written in upper case. We could put "Is" in upper case here, but I guess it is not quite right in English without a subject. I opted instead for removing the hyphen and making those items read like normal sentences, please let me know if you'd like a different option.
  • The paragraph about comments_count was worded in terms of columns of a Ruby class, but Ruby classes do not have columns. I just reworded a bit here.
  • The creation of the books table using the provided SQL has less columns, it is not creating the timestamps. I tried to reword the existing paragraph, but it was not very clear for my liking. I ended up moving the sentence about the SQL next to it, to then have more margin to describe the migration.
  • Model generation also generates a YAML file for the fixtures, and a dummy unit test.
  • Regarding "create will return the object and save it to the database", while that is correct, it is more natural for readers to have things enumerated in the order in which they happen: the method first saves the object, and then returns it.
  • Some code comments ended with a period, others didn't. I added periods were missing, following our guidelines.

The rest is self-explanatory.

What do you think?

@rails-bot rails-bot bot added the docs label Mar 15, 2024
Copy link
Member

@carlosantoniodasilva carlosantoniodasilva left a comment

Choose a reason for hiding this comment

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

👍 Thanks @fxn

@fxn fxn merged commit a534bac into main Mar 15, 2024
6 of 7 checks passed
@fxn fxn deleted the ar-guide-edits branch March 15, 2024 17:38
Comment on lines +227 to +230
That migration creates columns `id`, `title`, `author`, `created_at` and
`updated_at`. Each row of this table can be represented by an instance of the
`Book` class with the same attributes: `id`, `title`, `author`, `created_at`,
and `updated_at`. You can access a book's attributes like this:
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this. Listing all columns and attributes is more complete and clear. I went back and forth about how much to explain about how the created_at, updated_at and id are generated vs. the other columns.

@bhumi1102
Copy link
Contributor

What do you think?

👍 Thanks for the improvements @fxn. All of that makes sense. And thanks @carlosantoniodasilva for merging!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants