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

Fixed README #449

Merged
merged 2 commits into from
Jan 3, 2014
Merged

Fixed README #449

merged 2 commits into from
Jan 3, 2014

Conversation

seuros
Copy link
Collaborator

@seuros seuros commented Jan 1, 2014

This is the correct syntax since rails 3

@bf4
Copy link
Collaborator

bf4 commented Jan 1, 2014

What was wrong with the README? (BTW, it's the same in the UPGRADING file)

@seuros
Copy link
Collaborator Author

seuros commented Jan 2, 2014

The syntax was old.

@bf4
Copy link
Collaborator

bf4 commented Jan 2, 2014

Oh? Would you humor me with a source? Looks to me like it's still up-to-date? link.

Also, I don't know if this was intentional, but you removed the db:migrate task from running along with the migration installation.

@seuros
Copy link
Collaborator Author

seuros commented Jan 2, 2014

The source is the link you just posted . It say : "If you have multiple engines that need migrations copied over, use railties:install:migrations" in our case we have just one so we using the first syntax.

I re-added the db:migrate to the README but omitted it from UPGRADING.

IMO it bad practice to place a migration in-line

@bf4
Copy link
Collaborator

bf4 commented Jan 2, 2014

My thought regarding running the two tasks together is that it is the expected use case and not everyone knows you can do that. On the other hand, following my logic, the rake command should also run db:test:prepare. Although I suppose I should then add a SCOPE option

What are the downsides? This is only run in dev

@seuros
Copy link
Collaborator Author

seuros commented Jan 2, 2014

In a production env, the migrations are run automatically after the deploy.
While in dev, it will warn if there are migrations pending.

It is recommended to always look at the migration before doing them. Let say you added an index to some field before the gem maintainer released the update. Once you run the migration, it will fail because the index already exist, so a modification will be required.

Anyway, if you think i should add the db:migrate, i will do it.

@bf4
Copy link
Collaborator

bf4 commented Jan 2, 2014

Good point. Maybe then say install with... review the generated migrations... migrate?

@seuros
Copy link
Collaborator Author

seuros commented Jan 2, 2014

I rebased and repushed .

@bf4
Copy link
Collaborator

bf4 commented Jan 2, 2014

Whoa.. so many commits... maybe just

git checkout master && git checkout -b backup_master && git checkout - && git fetch upstream && git reset --hard upstream/master
# make your changes, cherry-pick, whatever and commit
git push -f origin master

@seuros
Copy link
Collaborator Author

seuros commented Jan 2, 2014

Thank you for your help @bf4

@@ -2,6 +2,8 @@ When upgrading

Re-run the migrations generator

rake railties:install:migrations FROM=acts_as_taggable_on_engine db:migrate
```shell
rake acts_as_taggable_on_engine:install:migrations
Copy link
Collaborator

Choose a reason for hiding this comment

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

So, this file is read in by the gemspec and displayed post-install. That's why I didn't use the markdown or the fences.

Also, I think I was just following a pattern already in the gemspec.

In any case, if this file is renamed, we'll need to change the code in https://github.com/mbleigh/acts-as-taggable-on/blob/master/acts-as-taggable-on.gemspec#L21

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think i will revert this change

bf4 added a commit that referenced this pull request Jan 3, 2014
Improve migration/upgrade instructions
@bf4 bf4 merged commit a3854ee into mbleigh:master Jan 3, 2014
tekniklr pushed a commit to tekniklr/acts-as-taggable-on that referenced this pull request Mar 19, 2021
Improve migration/upgrade instructions
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.

2 participants