titon / framework

A modular Hack framework that runs on HHVM.
http://titon.io
BSD 2-Clause "Simplified" License
206 stars 23 forks source link

Console package #76

Open alex-phillips opened 9 years ago

alex-phillips commented 9 years ago

Console package for building command line applications.

milesj commented 9 years ago

Added comments for basic syntax stuff. Still need to do another pass on the classes and architecture itself.

Could also use more tests granular unit tests :P

milesj commented 9 years ago

Made a second pass. Also a few things to think about:

1 - Instead of doing InputBag<Flag>, etc, all over the place, perhaps add a Titon\Console\Bag\FlagBag? And the same for arguments and options.

2 - Should we suffix the classes in Feedback, InputDefinition, System, UserInput with the type of class? For example, WindowsSystem instead of Windows. We use this everywhere, so maybe we should be consistent.

alex-phillips commented 9 years ago

1 - The reason I didn't go the route of FlagBag (and others) is because they would essentially be empty classes since they all share code. But that may not matter for code readability. I'll leave it up to you.

2 - I can definitely suffix the classes. Didn't consider that when I was initially writing them.

milesj commented 9 years ago

1 - I wonder if a type alias would work here instead of empty classes?

2 - Yeah let's just go ahead and suffix them.

alex-phillips commented 9 years ago

Made all of the changes so far. As far as the suffixing of the classes...

I'm going through and refactoring them all, but it seems extremely cumbersome when creating new commands (from the user's viewpoint). Ex: every time a new flag is created, the syntax new FlagInputDefinition(...) just seems like a lot to repeating. I know its been convention so far, but wouldn't the namespace already hint that its a Feedback class? But then again, we are using it for AsciiTree and MarkdownTree. So not sure when to consider using it and when NOT to.

Just a thought.

milesj commented 9 years ago

Good point. Maybe leave the InputDefinition classes without the suffix but apply the suffix to the other ones.

milesj commented 9 years ago

I've been thinking about this. Let's go ahead and integrate Kernel directly into the code. Console\Console should extend Kernel\Application, Console\Input should extend Kernel\Input, and Console\Output should extend Kernel\Output, etc. Generics should also be tied in.

If you get confused or anything, let me know.

alex-phillips commented 9 years ago

I've updated the AsciiTable class to use the suggested output styling (similar to npm list). Run demo to check it out.

milesj commented 9 years ago

Added a few more comments, we're almost there!

The major issue I have is that there aren't too many tests. Whenever I write tests, I try and be as granular as possible and test every method on every class. If you could somehow mock all the classes and test as much as possible, that would be great.

I'll also clone the repo and test it out on Windows again later today.

alex-phillips commented 9 years ago

@milesj, ping me when you're on IRC next. Can't seem to get the tests to run.