fuel / oil

Fuel PHP Framework - Fuel v1.x Oil command-line package
http://fuelphp.com/docs/packages/oil/intro.html
106 stars 67 forks source link

Fix Generate::migration() #180

Closed hackoh closed 11 years ago

hackoh commented 11 years ago

This fixes the root of https://github.com/fuel/oil/pull/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 commented 11 years ago

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: https://github.com/fuelphp/fuelphp/issues/24

hackoh commented 11 years ago

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

hackoh commented 11 years ago

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

WanWizard commented 11 years ago

Wasn't about your code. :smiley:

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 commented 11 years ago

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 commented 11 years ago

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

hackoh commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

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. (https://github.com/fuel/oil/pull/180#issuecomment-14882477) Then, would you consider merging?

WanWizard commented 11 years ago

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