Skip to content

Conversation

@azole
Copy link
Contributor

@azole azole commented Jun 22, 2015

No description provided.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Simply return RecordActionTemplate and remove templateName property?

@c9s c9s changed the title add action template Action template refactoring Jun 23, 2015
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we could name it as FileBasedActionTemplate

Copy link
Collaborator

Choose a reason for hiding this comment

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

And named the other as CodeGenActionTemplate since it's using CodeGen package to generate Actions

@c9s
Copy link
Collaborator

c9s commented Jun 23, 2015

也可以有另外一種做法是把這段邏輯包回 generator 中,然後把 CRUD 這個拿掉,讓 BaseRecordAction 直接自己用 generator,不過,看 test 中蠻多地方用到 CRUD::generate,所以不敢輕舉妄動的把它拿掉,因此選擇保留 CRUD,然後把要驛動的邏輯搬到 CRUD 裡。

OK 先把 CRUD 清掉,然後舊的 CRUD test 直接 drop 掉就好

Copy link
Collaborator

Choose a reason for hiding this comment

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

應該改成 registerAction 會比較好

Copy link
Collaborator

Choose a reason for hiding this comment

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

Use an UndefinedTemplateException ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

class UndefinedTemplateException extends LogicException

Copy link
Collaborator

Choose a reason for hiding this comment

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

這個好像可以拿掉

Copy link
Collaborator

Choose a reason for hiding this comment

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

should pass $this->className to str_replace, not cacheFile.

c9s added a commit that referenced this pull request Jun 24, 2015
Action template refactoring
@c9s c9s merged commit f170d4b into corneltek:master Jun 24, 2015
@c9s
Copy link
Collaborator

c9s commented Jun 24, 2015

我整理了一下: d6a1172

因為 ActionGenerate->generate 得回傳 GeneratedAction, 因此 cache 機制可能還是在 Runner 做比較恰趟,在 generate 裡面做 require 的動作變得不夠清楚,所以我後來還是把 cache 搬出來了。

要做在 ActionGenerate 裡面好像也是可以,但可能會是另外一個 method,來做 generate + load 這兩件事情。

@c9s
Copy link
Collaborator

c9s commented Jun 24, 2015

另外由於時間上來不及帶你做 rebase ,我先幫你把 changes 用 rebase 壓縮成一個 commit: 81e2a95

如果你如果要 pull changes,記得先 local 做 reset 到上一個 PR 的 commit,再做 pull:

git reset --hard e5b6d31ee83611cd49a643b7b2d218046b61ba7d
git pull c9s master 

再強制更新你的 GitHub repository

git push -f origin master

@c9s
Copy link
Collaborator

c9s commented Jun 24, 2015

另外 Exception 的地方需要分成 LogicException 跟 RuntimeException,我修改在這邊: cb806e0

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