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

Open option fixes #72

Merged
merged 2 commits into from
Mar 22, 2018
Merged

Open option fixes #72

merged 2 commits into from
Mar 22, 2018

Conversation

shellscape
Copy link
Contributor

This PR contains a:

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

Motivation / Use-Case

Supersedes #60. There was some parsing on the --open-app flag that wasn't correct in all scenarios, and parsing of --open-path that was incorrect. The code was also passing all of the open options blindly to opn, which could cause issues in the future. The tests were updated to more thoroughly check the open option's properties and how they are passed to opn.

Breaking Changes

None

Additional Info

N/A

update how open flags are used in code and applied to options
update cli description for --open-app
@codecov
Copy link

codecov bot commented Mar 22, 2018

Codecov Report

Merging #72 into master will increase coverage by 0.5%.
The diff coverage is 50%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master     #72     +/-   ##
========================================
+ Coverage   92.19%   92.7%   +0.5%     
========================================
  Files           7       7             
  Lines         269     274      +5     
========================================
+ Hits          248     254      +6     
+ Misses         21      20      -1
Impacted Files Coverage Δ
cli.js 96.77% <ø> (ø) ⬆️
lib/server.js 87.69% <100%> (ø) ⬆️
lib/options.js 89.65% <40%> (+1.85%) ⬆️

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 730452f...6433b7c. Read the comment docs.

@shellscape shellscape merged commit 8357793 into master Mar 22, 2018
@shellscape shellscape deleted the open-option-fixes branch March 22, 2018 17:04
@@ -46,7 +46,8 @@ const cli = meow(chalk`
--no-hot Instruct the client not to apply Hot Module Replacement patches
--no-reload Instruct middleware {italic not} to reload the page for build errors
--open Instruct the app to open in the default browser
--open-app The name of the app to open the app within
--open-app The name of the app to open the app within, or an array
containing the app name and arguments for the app

Choose a reason for hiding this comment

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

I think this argument requires more explanation. It needs to be clear this is a JSON array, not a JavaScript array (i.e. ['a', 'b', 'c',] wouldn't work). Otherwise the console warning could be confusing to users.

Choose a reason for hiding this comment

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

PR here

@benwiley4000 benwiley4000 mentioned this pull request Mar 22, 2018
7 tasks
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants