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

Fix Generate::migration() #180

Closed
wants to merge 2 commits into from

Conversation

hackoh
Copy link
Contributor

@hackoh hackoh commented Mar 13, 2013

This fixes the root of #177

Current magic migrations does not care of the case where the column name (or table name) contains underscores (or reserved words for example to, from and in).

In order to split the strict sense of the word, should not use underscores as concatenation character.
I propose the "." instead of the "".
But generating still allows "
" for compatibility.

The cases of current.

php oil g migration delete_is_visible_from_articles is_visible:bool
# The code generated doesn't specify table name.

php oil g migration add_to_mail_to_contact_to_users to_mail:varchar[255]
# The code generated doesn't specify table name.

This change enables the followings.

php oil g migration delete_is_visible_from_articles is_visible:bool
# The code generated is correct.

php oil g migration add.to_mail.to.contact_to_users to_mail:varchar[255]
# "." can be used to split the meaning

@WanWizard
Copy link
Member

The entire syntax is stupid and should be revised. Trying to fix things that are broken beyond repair is a waist of time imho... Added this issue to the requirements discussion for 2.0: fuelphp-storage/fuelphp#24

@hackoh
Copy link
Contributor Author

hackoh commented Mar 13, 2013

Which is you say "stupid syntax" about my codes? or is it "arguments syntax"?

@hackoh
Copy link
Contributor Author

hackoh commented Mar 13, 2013

If you meaned the first, I try to fix them.Else, I would join your discus.

@WanWizard
Copy link
Member

Wasn't about your code. 😃

i think php oil g migration add.to_mail.to.contact_to_users to_mail:varchar[255] is a stupid way of specifying your generation requirements.

@hackoh
Copy link
Contributor Author

hackoh commented Mar 14, 2013

I propose the following.

php oil g migration create articles title:varchar[255] content:text created_at:int
php oil g migration rename_table articles posts
php oil g migration add title articles title:varchar[255]
php oil g migration delete title articles
php oil g migration rename_field created_at posted_at articles
php oil g migration drop articles

@WanWizard
Copy link
Member

Better, but still not really flexible, especially not if you have a table with 40 columns...

@hackoh
Copy link
Contributor Author

hackoh commented Mar 14, 2013

Hmm...

How is the approach like the xargs?

# exmple
cat | php oil migration create articles -x
title:varchar[255]
content:text
created_at:int
updated_at:int
# ctrl + d
     Creating migration: /path/to/001_create_article.php

# this is emulating the below.
# cat | xargs php oil g migration create articles

In consideration of the environment where the xargs is not installed, the option which carries out xargs conversion of the standard input is added to the oil command.

What do you think?

@WanWizard
Copy link
Member

That won't work on Windows (and maybe other platforms too). The best bet I think is to create some sort of shell, in which you can type commands like add field title varchar[255]. But that's quite a lot of work, and should be part of the 2.0 effort of redesigning oil.

@hackoh
Copy link
Contributor Author

hackoh commented Mar 14, 2013

The best bet I think is to create some sort of shell, in which you can type commands like add field title varchar[255].

Does it mean a thing like an interactive tool (for example, oil console)?

@hackoh
Copy link
Contributor Author

hackoh commented Mar 14, 2013

If so, it will be a big work as your saying.

However, we need the small fix of this issue for 1.x.
I would fix this branch to my proposing syntax. (#180 (comment))
Then, would you consider merging?

@WanWizard
Copy link
Member

@FrenkyNet your opinion? I don't like the dots at all, but don't see any other quick fix for this issue...

@WanWizard WanWizard closed this Jun 11, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants