Skip to content
This repository has been archived by the owner on Apr 8, 2019. It is now read-only.

Explain --open-app more #73

Closed
wants to merge 1 commit into from
Closed

Explain --open-app more #73

wants to merge 1 commit into from

Conversation

benwiley4000
Copy link

@benwiley4000 benwiley4000 commented Mar 22, 2018

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update
  • docs update

Motivation / Use-Case

Followup to my comment here. Makes the --open-app argument a bit easier to use correctly.

Followup to my comment [here](#72)
@benwiley4000 benwiley4000 mentioned this pull request Mar 22, 2018
6 tasks
@codecov
Copy link

codecov bot commented Mar 22, 2018

Codecov Report

Merging #73 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master     #73   +/-   ##
======================================
  Coverage    92.7%   92.7%           
======================================
  Files           7       7           
  Lines         274     274           
======================================
  Hits          254     254           
  Misses         20      20

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8357793...cc2ee7f. Read the comment docs.

@shellscape
Copy link
Contributor

Rejecting this for a number of reasons:

  1. you didn't update the actual CLI
  2. we don't need that level of verbosity in the CLI output when the README exists
  3. The link should go into the README under the open option. (the README option still needs to be updated with the array info)
  4. You don't need to pass a JSON array, it'll be interpolated as a string from the CLI via meow and parsed normally:
JSON.parse('["a", "b", "c"]')
// --> [ 'a', 'b', 'c' ]

@shellscape shellscape closed this Mar 22, 2018
@benwiley4000
Copy link
Author

Didn't know about the meow thing! Thanks. I find the concept of passing an array to a cli puzzling, but if it works, it works!

# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants