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

Rework options description to clarify option types and custom processing #953

Merged
merged 4 commits into from
May 8, 2019

Conversation

shadowspawn
Copy link
Collaborator

Mainly rewriting options coverage in README:

Still to come: small changes to version option description

Comments welcome from anyone.

@shadowspawn shadowspawn changed the title WIP Rework options description to clarify option types and custom processing WIP: Rework options description to clarify option types and custom processing Apr 27, 2019
@shadowspawn shadowspawn marked this pull request as ready for review April 29, 2019 09:58
@shadowspawn shadowspawn changed the title WIP: Rework options description to clarify option types and custom processing Rework options description to clarify option types and custom processing Apr 29, 2019
@shadowspawn
Copy link
Collaborator Author

I have added about 100 lines to length of README, but hopefully clearer!

(I didn't refactor the regular expression support in with the rest of the options. Future work.)

(I think we should add a table of contents, but after tidying more of the README so the headings make better sense. Future work.)

@ozbillwang
Copy link

Looks much better. 👍

@shadowspawn
Copy link
Collaborator Author

I had a look at "Insights > Traffic" and am surprised how much traffic examples/ gets. In last 2 weeks the README got 2912 and examples/ got 518 unique visitors.

Since the examples are being used, I think I'll add some more info to the new example files I added like "as shown in README", and add the expected output in a comment. However, this PR can be reviewed or merged before I do that, as examples can be added separately.

@ozbillwang
Copy link

ozbillwang commented May 6, 2019

One suggestion.

.option('-c, --cheese <type>', 'add the specified type of cheese', 'blue');

Until now, I still don't fully understand how this option <type> to be used.

I accidentally wrote a code as below (without <type>)

 .option('-c, --cheese', 'add the specified type of cheese', 'blue');

Then this option doesn't work as expected.

But I am not sure what should be put, currently just copy it as <type>. There is no any sample to change this part in README

@shadowspawn
Copy link
Collaborator Author

shadowspawn commented May 6, 2019

Thanks @ozbillwang. I have in the early description:

The two most used option types are a boolean flag, and an option 
which takes a value (declared using angle brackets)

but do not clarify when describing default values:

You can specify a default value for an option which takes a value.

Is it mainly the default values case you tripped over? Or do you think it would be helpful to expand on "takes a value" in both places?

@shadowspawn
Copy link
Collaborator Author

(Updated examples to include README reference and example output.)

@shadowspawn shadowspawn requested a review from abetomo May 8, 2019 10:15
Copy link
Collaborator

@abetomo abetomo left a comment

Choose a reason for hiding this comment

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

Thank you! 👍

@abetomo abetomo merged commit 9b6fdae into tj:master May 8, 2019
@shadowspawn shadowspawn deleted the feature/943-options branch May 30, 2019 09:36
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.

3 participants