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

Updates to the Migration Guide #8375

Merged
merged 1 commit into from
Dec 3, 2012
Merged

Conversation

steveklabnik
Copy link
Member

I'm going to be taking a look at all of the Guides as we get closer to the Rails 4 release. I've done a bunch of work in docrails already, but with these bigger changes to the Guides, I figured that a pull request would be best.

Here are some initial thoughts on improving the Migration Guide. Most of the content changes are at the beginning. Obviously, this will probably be squashed before it's merged.

/cc @fxn @vijaydev

@barelyknown
Copy link

The intro is a very nice improvement.

It might make sense to add some content about using migrations to seed data vs. seeds.rb vs. other approaches. I hear that talked about a decent amount and it seems like the "rails way" is to use seeds.rb, so maybe that deserves mention. I didn't see it anywhere in the guide, so apologies if it is and I missed it.

creating extra work for yourself and your co-workers and cause major headaches
if the existing version of the migration has already been run on production
machines. Instead, you should write a new migration that performs the changes
you require. Editing a freshly generated migration that has not yet been
Copy link
Member

Choose a reason for hiding this comment

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

Just a detail but there are two spaces just after the dot (even on the line 605). :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ugh. That was left from before. I'll fix those.

@steveklabnik
Copy link
Member Author

@barelyknown that would be a good idea, yes. there's nothing about this currently.

Active Record provides methods that perform common data definition tasks in a
database independent way (you'll read about them in detail later):

* `add_column`
Copy link
Member

Choose a reason for hiding this comment

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

This list is useful to me. I always forget what are the supported methods and I always look this guide.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm always torn: guides should have just the most useful stuff, not have a copy of everything in the API docs. Maybe there's a better balance to be struck here.

Copy link
Member

Choose a reason for hiding this comment

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

It is true. Maybe I'm looking in the wrong place ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand why you're torn about this, but this makes it feel complete. One gets a better understanding of what DDL changes are possible with migrations. At the very least, please retain a link to the relevant API documentation.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I'll put an API doc link. we should use more of those.

Copy link
Member

Choose a reason for hiding this comment

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

There is something solvable but unsolved, which is a way to link to the API (or viceversa) that links to edge in edge, and to stable in stable.

Absolute or relative paths work out of the box for in-guides or in-API links, but we need to invent something for cross-references.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ahhh. Yeah.

@steveklabnik
Copy link
Member Author

Updated to fix both the spacing issue and the db/seeds issue.

@steveklabnik
Copy link
Member Author

Rebased and squashed. Unfortunately, GitHub still says no merge even though locally, it looks fine? Hm. :(

Still not 100% sure what to do about the API issue. Maybe it's best to leave them in for now.

This introduces a bunch of editing/re-writing to the migrations guide.
There were a bunch of small changes, and a few larger ones:

* mysql does support transactions.
* Add a section about db/seeds.rb
* Largely re-wrote the first few sections
@steveklabnik
Copy link
Member Author

Merge-able now.

@fxn
Copy link
Member

fxn commented Dec 3, 2012

Nice simplification, I like the new wording.

We need to resolve the cross-linking somehow because for completeness the guide should link IMO to listings of column types and macros. Let me think about it.

But that is not a blocker, let's merge.

fxn added a commit that referenced this pull request Dec 3, 2012
@fxn fxn merged commit c9c1e6b into rails:master Dec 3, 2012
@steveklabnik
Copy link
Member Author

🤘 thanks!

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.

7 participants