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

fix: errors quote #915

Closed
wants to merge 2 commits into from
Closed

fix: errors quote #915

wants to merge 2 commits into from

Conversation

zce
Copy link
Contributor

@zce zce commented Feb 14, 2019

- "error: missing required argument `%s'"
+ "error: missing required argument '%s'"

@kira1928
Copy link
Contributor

@zce
Your pr broke the lint test.
https://travis-ci.org/tj/commander.js/jobs/493226140#L464-L478
I think those single quotes were inserted to cheat eslint, which is a bad practice :(
Could you change the https://github.com/tj/commander.js/blob/master/.eslintrc.json file to allow only double quotes or change all those strings to be wrapped by single quotes?
(BTW, I personally prefer double quotes :p)

@shadowspawn
Copy link
Collaborator

I have some comments too @zce. Sorry, you probably through it was an easy pull request!

  1. What problem are you fixing? Personally I do not like how the left/right quotes look and it is unusual, is that what you are fixing? e.g. currently
error: missing required argument `text'
  1. what cli commands have you looked at? The examples I found all used straight single quotes:
$ git silly 
git: 'silly' is not a git command. See 'git --help'.
$ hg silly
hg: unknown command 'silly'
$ cc silly
clang: error: no such file or directory: 'silly'
  1. An observation for how much this affects people (i.e. semver). The change does not affect functionality, but some people will have designed their own error commands to match this style and will care if we change the style.

@zce
Copy link
Contributor Author

zce commented Mar 17, 2019

error: missing required argument `text'

`text' starts with right quotes, but ends with straight single quotes

@shadowspawn
Copy link
Collaborator

I found an OLD reference to this very issue: https://www.cl.cam.ac.uk/~mgk25/ucs/quotes.html

I would prefer

'unexpected'

rather than

`unexpected`

(although I have to admit I am now recognise the left quote from markdown, but keep it simple!)

@shadowspawn
Copy link
Collaborator

Although this does not change the functionality, some people will have written their own error messages to match commander style (so I suggest at least semver:minor), and some smaller number may be parsing error messages to do custom handling (so we could be paranoid and include in a semver:major).

Consider whether you think semver patch or minor or major when you review @abetomo or @vanesyan, and comments welcome from any readers.

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!
I think 'unexpected' is good.

Copy link
Collaborator

@shadowspawn shadowspawn left a comment

Choose a reason for hiding this comment

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

Oops, I forgot to formally review this myself! Thanks @abetomo

Thanks for earlier changes @zce. Looks good to me.

@shadowspawn shadowspawn removed the request for review from roman-vanesyan June 22, 2019 02:02
@shadowspawn shadowspawn added this to the v3.0.0 milestone Jun 22, 2019
@shadowspawn
Copy link
Collaborator

Merging into release/3.0.0 branch.

@shadowspawn shadowspawn added the pending release Merged into a branch for a future release, but not released yet label Jun 23, 2019
@shadowspawn
Copy link
Collaborator

Already had two approvals. Merged into v3 and will be included in that release. Closing to make it clear that should not be merged into master.

Thank you for your contributions.

@zce zce deleted the fix-errors-quote branch June 24, 2019 11:06
@shadowspawn
Copy link
Collaborator

Available now as a prerelease. See #1001

@shadowspawn
Copy link
Collaborator

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.

None yet

4 participants