search

gruntjs / grunt-cli

Grunt's command line interface.
http://gruntjs.com/
MIT License
706 stars 248 forks source link

Use shared grunt-known-options #103

Closed shama closed 8 years ago

shama commented 8 years ago

Fixes GH-102

This uses the newly created https://github.com/gruntjs/grunt-known-options for sharing the known options between Grunt and grunt-cli. This makes the previous known Grunt options syntax backwards compatible: grunt --gruntfile some/Gruntfile.js vs grunt --grunt=some/Gruntfile.js. Both will work with this PR rather than just the later.

Unknown options in users Gruntfiles will still need to be aware of the new syntax though.

/cc @vladikoff @Arkni @EtienneMiret

Arkni commented 8 years ago

The base option still need an alias (mentioned in docs). Otherwise, LGTM :)

Arkni commented 8 years ago

@shama Have you considered moving the code, that parse the grunt options into a form that nopt can handle, to the newly created repo ? because the same code is used in gruntjs/grunt.

tkellen commented 8 years ago

I'm not as close to this as ya'll are these days but do you need to take versioning into account @shama? I imagine grunt-cli will eventually need to support different sets of options per-version.

shama commented 8 years ago

@Arkni Thanks! I'll make those changes.

@tkellen Yep! @vladikoff and I chatted a little about it. The plan is to keep this as light as possible. Follow semver with releases, ensuring the latest works with the latest of Grunt. If a user's specific project is incompatible with the latest release of grunt-cli, the recommendation is to install the version of grunt-cli that works to the local project and use npm scripts to call grunt, until they can update.

shama commented 8 years ago

@Arkni I'm changing my mind about moving that code to grunt-known-options. I like that the library just exports a raw object of info about the options, rather than a specific format for nopt. The API got awkward trying to do both. Because require('grunt-known-options') would also have aliases and known even though they are not grunt options, just for nopt. I thought about moving into another file or moving the options into their own API, i.e: require('grunt-known-options').getOptions() which didn't feel right either... plus adding code to it means we'd need tests... anyways, I'm fine having that code in both places until it's replaced with: https://github.com/gruntjs/rfcs/pull/2

@vladikoff Please review and merge when you get a chance! Thanks!

Arkni commented 8 years ago

I was thinking that moving those lines of code to grunt-known-options will simplify the interaction with the cli.

@Arkni I'm changing my mind about moving that code to grunt-known-options. I like that the library just exports a raw object of info about the options, rather than a specific format for nopt

I completely agree with you. Adapting it to use nopt will make it useless in case someone want to work on Grunt known options directly.

The API got awkward trying to do both. Because require('grunt-known-options') would also have aliases and known even though they are not grunt options, just for nopt. I thought about moving into another file or moving the options into their own API, i.e: require('grunt-known-options').getOptions() which didn't feel right either... plus adding code to it means we'd need tests... anyways, I'm fine having that code in both places until it's replaced with: gruntjs/rfcs#2

Non, I'm fine with the way it is right now. I'm just making an observation that was totally wrong in the context of re-usability.

Thanks for the clarification :)