Massive Refactoring and an official v0.2.0 release, if you'll take it. #3
Conversation
…l for backwards compatibility.
…eated for this repository.
| require_once '../vendor/autoload.php'; | ||
|
|
||
| // define CLI arguments | ||
| $cli = new Commando\Command(); |
There was a problem hiding this comment.
Just asking what would be the downside to just getting the args instead of using this dependencie?
There was a problem hiding this comment.
I was pretty hesitant at first at adding another library, but this has support for built in error handling/exit codes, support for arguments without a name, aliases, even a help menu; and eventually, I can easily add support for templates and label mappings (--template and --labels). These were all features I was going to add to the original, but I currently don't have the time to implement all this at the moment. This library was a shortcut until I can get around to writing an implementation that doesn't require a third party library (in other words, this dependency will be removed in a later version).
|
Thanks @nbish11 - sry when i comment on things i wrote years ago :) can't seperate though |
| @@ -1,242 +1,113 @@ | |||
| <?php | |||
|
|
|||
| // "autoload" our classes | |||
| if (!class_exists('ins0\GitHub\Repository')) { | |||
| require_once 'src/Repository.php'; | |||
There was a problem hiding this comment.
would suggest to add __DIR__ to avoid chdir conflicts and a possible bc break duo to missing autoloading. to be hornest i don't like classes that have a autoload within the class itself. Whats your opinion on this?
1# Ship the class "as it" - user should deal with the autoloading (eg. composer or user itself)
2# Or this autoload
These days 1# is common, don't see much 2# in newer classes
There was a problem hiding this comment.
I agree classes should not contain code like this. I can add these files to Composer's autoloading section for people who need the backwards compat and are using Composer, and if the user isn't using Composer then they can manually require themselves.
There was a problem hiding this comment.
@nbish11 That would lead to a BC break as the class then uses classes that wouldn't be loaded without user interaction (installing from composer or autoloading itself). Maybe v1 is born then
There was a problem hiding this comment.
Completely forgot about that. It's up to you whether or not I should break compat. Let me know and I'll rewrite the diff.
|
@m1guelpf I've been away for roughly a year, so I haven't had the chance to follow through with this request. In either case, I still don't know what Marco plans on doing with this PR. |
|
@nbish11 my bad, got alot of work in the past year so i couldn't work for the os community. I prepare this pr right now to get merged. Sry for the long time :/ |
|
Glad to hear!! |
I have tried to keep a clean commit history without over-populating all the changes I've made. The commits are named after the major change being applied to each file/folder.