Skip to content

Conversation

@rozele
Copy link
Contributor

@rozele rozele commented Aug 2, 2016

The high level react-native --help command works okay, but using react-native command --help seems to be broken. It works for some commands that have basic usage (i.e., options only, including run-android and run-ios), but it does not work for commands that have optional arguments in their usage, like react-native link --help.

This fixes commands like react-native link, although it deletes some behavior that I'm not quite sure what the original intent was (cc author @grabbou).

The high level `react-native --help` command works okay, but using `react-native command --help` seems to be broken. It works for some commands that have basic usage (i.e., options only, including run-android and run-ios), but it does not work for commands that have optional arguments in their usage, like `react-native link --help`.

This fixes commands like `react-native link`, although it deletes some behavior that I'm not quite sure what the original intent was (cc author @grabbou).
@ghost
Copy link

ghost commented Aug 2, 2016

By analyzing the blame information on this pull request, we identified @grabbou and @janicduplessis to be potential reviewers.

@ghost ghost added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Aug 2, 2016
@rozele
Copy link
Contributor Author

rozele commented Aug 2, 2016

@grabbou definitely want your input here. I deleted this bit:

const usage = this.usage();    

if (usage !== '[options]') {    
  const formattedUsage = usage.map(    
    example => `    ${example.desc}: \n    ${chalk.cyan(example.cmd)}`,    
  ).join('\n\n');    

  output = output.concat([    
  chalk.bold('  Example usage:'),    
    '',    
    formattedUsage,    
  ]);    
}  

Because I didn't know what you were trying to do. this.usage() should always return a string, so I don't know why you would apply a map operation to it, maybe you meant some other function to extract examples from a command?

@ghost ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 2, 2016
@Kureev
Copy link
Contributor

Kureev commented Aug 2, 2016

It meant to display a proper --help message with examples. Not sure if it works after merge (we checked only command functionality). Probably a better way would be to fix an original behavior.

@rozele
Copy link
Contributor Author

rozele commented Aug 2, 2016

@Kureev Do you know how to add examples to a commander Command instance? I didn't see anything of the sort in the commander documentation.

@ghost ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 2, 2016
@Kureev
Copy link
Contributor

Kureev commented Aug 2, 2016

We added them as a help (usage) message. Every command has a structure, described here. So the mapping you removed is for the example usage message. It formats a nice message and outputs it as a "usage" info.

@rozele
Copy link
Contributor Author

rozele commented Aug 3, 2016

@Kureev okay, I'll publish another update, I think it may have just been a bug, where the output from usage was being used instead of examples.

@ghost ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 3, 2016
@facebook-github-bot
Copy link
Contributor

@rozele updated the pull request.

@ghost ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 3, 2016
@rozele
Copy link
Contributor Author

rozele commented Aug 3, 2016

@Kureev I added what I expect is the correct behavior for examples, however, there are currently no documented react-native commands that have examples. Also, that exported type is never being used. The architecture is currently using the Command prototype from commander (https://github.com/tj/commander.js/blob/master/index.js#L82), which has no examples. When you parse the declaration of each command, even if it has an examples property of the form { desc: string, cmd: string } it will not be copied into the Commander command object which is ultimately what gets used.

At this point, I would actually recommend reverting my second commit and just deleting the examples stuff as I had in the first commit. cc @grabbou

@ghost ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 3, 2016
@grabbou
Copy link
Contributor

grabbou commented Aug 3, 2016

@rozele currently run-ios has examples, we decided that it's better for users to provide explicit array of examples instead of showing the default commander output which in 99% cases was just [options] (string).

We were never adding custom usage to commander (not implemented in rnpm) so every time the output of this.usage() was options.

Now, to make it easier, we pass an array of examples to .usage(..) and that's why it's being an array.

Later, we check if it's !== '[options]' (if not, it's an array) and we map examples to a readable output.

Check the example docs here https://github.com/facebook/react-native/blob/master/local-cli/runIOS/runIOS.js#L84-L93

Here's how it looks like when run with --help:
screen shot 2016-08-03 at 09 07 39

@ghost ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 3, 2016
@ghost ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 3, 2016
@geof90 geof90 mentioned this pull request Aug 3, 2016
When an `examples` property is present, the command is assumed to not take any additional command line arguments (i.e., the usage is `[options]` only), and the return value of `.usage()` is an array of examples.

This provides a proper check for the result of `cmd.usage()` to be a string (the normal case when examples are not present), in which case the set of examples are not processed.

Prior to this change, running `react-native link --help` would result in an exception, because the `.usage()` output for `link` is `[options] [packageName]` (note, !== `[options]`).

In general, overriding the behavior of usage() may be limiting, as this change will not support adding examples to `react-native link --help` while still allowing the first line of the --help message to include the original usage (`[options] [packageName]`).
@facebook-github-bot
Copy link
Contributor

@rozele updated the pull request.

@ghost ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 3, 2016
@ghost
Copy link

ghost commented Aug 3, 2016

@rozele updated the pull request.

@rozele
Copy link
Contributor Author

rozele commented Aug 3, 2016

@grabbou, check the description of the most recent commit in this PR, specifically:


This provides a proper check for the result of cmd.usage() to be a string (the normal case when examples are not present), in which case the set of examples are not processed.

Prior to this change, running react-native link --help would result in an exception, because the .usage() output for link is [options] [packageName] (note, !== [options]).

In general, overriding the behavior of usage() may be limiting, as this change will not support adding examples to react-native link --help while still allowing the first line of the --help message to include the original usage ([options] [packageName]).


const usage = this.usage();
const usageIsString = typeof(usage) === 'string';
const usageString = usageIsString ? usage : '[options]';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's going to be an issue with this eventually. For example, how does one add an examples section to the link command? If you did add examples, you'd lose the original usage() value ([options] [packageName]).

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 3, 2016
@grabbou
Copy link
Contributor

grabbou commented Aug 3, 2016

Thanks @rozele for the explanations, turns out I kinda misunderstood the original issue. I'll close this one in favour of #9185 which is hopefully fixing this problem

@grabbou grabbou closed this Aug 3, 2016
@rozele rozele deleted the commandHelp branch August 3, 2016 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants