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.

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

Automate conversion of options to arguments #70

Merged
merged 12 commits into from
Jul 16, 2014

Conversation

dylancwood
Copy link
Collaborator

As discussed in PR #61, this pull request creates a more structured system for converting task-options to CLI flags and arguments. After I tweaked my original solution to accommodate the various use cases, I decided that it would be easiest to put the bulk of the logic in a separate module that is reusable by other projects.

This PR uses flopmang, a module which I wrote just for this purpose. It is fully unit-tested, and I encourage you to look over the code when you have a chance: https://github.com/dylancwood/flopmang.

I tried to keep the changes here small in scope by not modifying any of the tests (except one, see notes in-line) or adding any new options. Hopefully this gives you high confidence that the changes do not affect the functionality of the code.

Once this is merged (when you approve, of course), it should be even easier for me and other collaborators to add more tasks and options to continue expanding this module's coverage of the git CLI.

Please let me know if you have any questions or concerns.

@@ -2,65 +2,78 @@

var async = require('grunt').util.async;
var grunt = require('grunt');
var ArgUtil = require('flopmang');
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the new module that I created.

@rubenv
Copy link
Owner

rubenv commented Jul 14, 2014

Really good stuff. The fact that the test suite keeps passing (aside from two minor details, but they're OK) despite this big change is a testament to the quality of it.

I think you'll agree that this is a huge change, not something we can push as an update to 0.2.x.

Unless there's a good reason not to, I suggest we merge this to master and then work at ensuring #44 is fine, to lead up to a new big release.

@dylancwood
Copy link
Collaborator Author

That sounds good.

We can also make a non -production release, and ask users to test with their Grunt workflows in order to catch any bugs that might have been missed by the unit tests.

@dylancwood
Copy link
Collaborator Author

I'm glad that you like it.

I'm happy to help with #44, but adding all git options will be a tedious task. Would you be willing to split up the git commands?
Thanks!

@rubenv
Copy link
Owner

rubenv commented Jul 14, 2014

I'm happy to help with #44, but adding all git options will be a tedious task. Would you be willing to split up the git commands?

We don't need all of them, I only really care about throwing out the ones where we differ and keeping the ones we already support. Any missing flags can be added later on as future improvements.

@dylancwood
Copy link
Collaborator Author

That sounds good. I will start working on #44 after this gets merged into master.

rubenv added a commit that referenced this pull request Jul 16, 2014
Automate conversion of options to arguments
@rubenv rubenv merged commit 1e26f5b into rubenv:master Jul 16, 2014
@rubenv
Copy link
Owner

rubenv commented Jul 16, 2014

Merged! Fantastic work.

Now that you've written most of the module (in its current state), would you be interested in co-maintaining it? I'll add you to the git repo, so you can commit to it directly for small fixes.

Do send PRs for bigger changes / things you're unsure of (or need a second pair of eyes on). And just ping me to roll releases.

Psyched, this is a big step forward for the module.

@dylancwood
Copy link
Collaborator Author

Thanks @rubenv! I'm psyched as well. I am happy to accept your offer of co-maintaining this library, and will still continue to use PRs for most changes: I'm a firm believer in code review and audit trails. I'll try to keep each PR small from now on so that it is quick to review.

@rubenv
Copy link
Owner

rubenv commented Jul 16, 2014

👍

@rubenv rubenv mentioned this pull request Jul 18, 2014
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