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.

Sign up for GitHub

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

Jump to bottom

Command autoloading feature #49

Merged
merged 9 commits into from Dec 25, 2014
Merged

Command autoloading feature #49

merged 9 commits into from Dec 25, 2014

Conversation

dh3014
Copy link
Contributor

@dh3014 dh3014 commented Dec 23, 2014

This PR actually mix with 3 stuffs.

  • Command autoloading feature
  • A bug fix when throwing an ExecuteMethodNotDefinedException
  • make ProgramName of an Application be its sufficient part of $argv[0] only

I can re-organize it if you want.

@c9s
Copy link
Owner

c9s commented Dec 24, 2014

Looks great, but seems the build failed on travis-ci, can you take a look?

c9s
c9s reviewed Dec 24, 2014
View reviewed changes
src/CLIFramework/Application.php
@@ -165,6 +165,7 @@ public function loadTopic($topicId) {
public function init()
{
// $this->addCommand('list','CLIFramework\\Command\\ListCommand');
parent::init();
Copy link
Owner

Choose a reason for hiding this comment

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

looks good

@dh3014
Copy link
Contributor Author

dh3014 commented Dec 24, 2014

Here's the current draft after some modification.

[https://github.com/dh3014/CLIFramework/commit/10139c9519ae039346140a36e21302e21c894b68]

@dh3014
Copy link
Contributor Author

dh3014 commented Dec 25, 2014

Latest draft, added commandAutoloadEnabled property in Application

[https://github.com/dh3014/CLIFramework/commit/f290d755c17a449a10bf15167324023fb79a90e4]

@dh3014
Copy link
Contributor Author

dh3014 commented Dec 25, 2014

Oops, there's a bug in the previous commit,
provides a callback with arity 2 will make array_walk behaves different.

see [https://github.com/dh3014/CLIFramework/commit/ebcfab597db4e9bcb449c383ccc277270906baf6]

@c9s
Copy link
Owner

c9s commented Dec 25, 2014

You can push your changes to master branch to update commits to this PR.

@dh3014
Copy link
Contributor Author

dh3014 commented Dec 25, 2014

pushed to master branch

@c9s
Copy link
Owner

c9s commented Dec 25, 2014

Looks great! there are only few things we need to improve, then we can merge. ; )

@dh3014
Copy link
Contributor Author

dh3014 commented Dec 25, 2014

Latest commit: [https://github.com/dh3014/CLIFramework/commit/38aff0ac3292085ed81e91e7555523d81a6fc710]

  • Remove the $path property from CommandAutoloader
  • Rewrite extension removal method

@c9s
Copy link
Owner

c9s commented Dec 25, 2014

Thanks! I think we can then roll out a new release for this feature.

c9s added a commit that referenced this pull request Dec 25, 2014
@c9s
Merge pull request #49 from dh3014/master
88fe15a 
Command autoloading feature
@c9s c9s merged commit 88fe15a into c9s:master Dec 25, 2014
@c9s c9s mentioned this pull request Feb 1, 2015
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@dh3014 @c9s