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

Ensure valid migration filename on generating migration #7419

Merged
merged 1 commit into from Sep 6, 2012
Merged

Ensure valid migration filename on generating migration #7419

merged 1 commit into from Sep 6, 2012

Conversation

releu
Copy link
Contributor

@releu releu commented Aug 22, 2012

rails g migration add_statistic_updated_at:datetime

create    db/migrate/20120822103649_add_statistic_updated_at:datetime.rb

rake db:migrate

llegal name for migration file: ...db/migrate/20120822103649_add_statistic_updated_at:datetime.rb
    (only lower case letters, numbers, and '_' allowed)



unless @migration_file_name =~ /^[_a-z0-9]+$/
raise ActiveRecord::IllegalMigrationNameError.new(@migration_file_name)
Copy link
Contributor

Choose a reason for hiding this comment

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

We should not reference ActiveRecord from the Rails directory. Or we should move this validation to the Active Record migration generator or provide a similar error in Rails. Given that this validation is specific to Active Record, I would do the former.

@releu
Copy link
Contributor Author

releu commented Aug 22, 2012

Thanks.

I have moved validation to AR. Is it ok to use AR::IllegalMigrationNameError in tests?

@jeroeningen
Copy link

It is nice to raise an error if the name of the migration is incorrect.
But on the other side I think it's a little overkill...

@releu
Copy link
Contributor Author

releu commented Aug 28, 2012

So.. Merge or Close?

@route
Copy link
Contributor

route commented Aug 28, 2012

I'm 👍, just squash your commits into one and rebase, otherwise it will be the next request ;)

@steveklabnik
Copy link
Member

Yep, both those will need done.

@releu
Copy link
Contributor Author

releu commented Aug 28, 2012

All done

@homakov
Copy link
Contributor

homakov commented Aug 28, 2012

👍 Also I just want rails putthingsback command that will revert just created crap

@steveklabnik
Copy link
Member

Somehow, it's out of date again.

@releu
Copy link
Contributor Author

releu commented Aug 28, 2012

I don't understand what is wrong. What should I do?

@steveklabnik
Copy link
Member

Rebase it again. This happens often when CHANGELOGS get merged, they tend to cause a conflict.

@releu
Copy link
Contributor Author

releu commented Aug 28, 2012

Rebased

@homakov
Copy link
Contributor

homakov commented Aug 29, 2012

@steveklabnik you don't see a green button right? Btw why github doesn't show other people that this PR needs to be rebased?

@releu
Copy link
Contributor Author

releu commented Aug 29, 2012

Another one rebase :)

@steveklabnik
Copy link
Member

Btw why github doesn't show other people that this PR needs to be rebased?

No idea. You should tell them. ;)

@releu
Copy link
Contributor Author

releu commented Aug 29, 2012

Is it unavailable again?

@steveklabnik
Copy link
Member

Oh, no, you're good. I'm not allowed to merge, or I would. You'll have to wait for someone on core.

@releu
Copy link
Contributor Author

releu commented Sep 6, 2012

bump!

josevalim pushed a commit that referenced this pull request Sep 6, 2012
…nerator

Ensure valid migration filename on generating migration
@josevalim josevalim merged commit 75bde02 into rails:master Sep 6, 2012
sikachu added a commit to sikachu/rails that referenced this pull request Sep 6, 2012
carlosantoniodasilva added a commit that referenced this pull request Sep 6, 2012
Update Active Record CHANGELOG for #7419 [ci skip]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants