[WIP] Help style #164

K3TH3R · 2017-10-02T04:38:51Z

@kentcdodds this isn't quite ready as I'm running into some issues with tests, but I didn't want to fill up the GitHub issue with back and forth. On several of the help-related tests I'm getting the following error: TypeError: Cannot read property 'help-style' of undefined. I'll use one of the tests as an example:

test('if there is a help script in the psConfig, does not show the help', () => {
  mockBinUtils.mock.psConfig = {scripts: {help: 'hi'}}
  expect(parse('help')).not.toBe(undefined)
  expect(mockGetLogger.mock.info).toHaveBeenCalledTimes(0)
})

If I change that to this, the test stops erroring:

test('if there is a help script in the psConfig, does not show the help', () => {
  mockBinUtils.mock.psConfig = {scripts: {help: 'hi'}, options: {}}
  expect(parse('help')).not.toBe(undefined)
  expect(mockGetLogger.mock.info).toHaveBeenCalledTimes(0)
})

However, I'm under the impression that arbitrarily adding the options object to the config is probably against best testing practices?

The tests are specifically choking on this line:

const helpStyle = String(psConfig.options['help-style'] || 'all')

Is the only way to solve this by writing code checking whether the options property exists? It just seems a bit dirty/weird since I don't see anywhere else where you're checking for existence of options nor the scripts property, like maybe I'm not grok'ing something I should be?

kentcdodds · 2017-10-02T06:13:20Z

Thanks so much! I'm fine with the solution you mentioned. Could you also rebase your branch in master?

K3TH3R · 2017-10-03T07:50:14Z

@kentcdodds Okay, so I have all of the tests passing, but I have a code coverage issue that I'm not sure how to fix. The coverage tool is complaining that these lines aren't tested, but I'm not entirely sure how to write a test for that block of code. Parser doesn't expose the help function and I'm still rather new to writing tests, so any help or guidance would be much appreciated.

… pattern

* Update concurrently => concurrent in examples * Add myself as a contributer * Add myself as a contributer properly, this time :)

kentcdodds

Looking good! Just a few comments :)

I updated your branch, so make sure to pull the latest changes.

kentcdodds · 2017-10-04T01:37:21Z

src/bin-utils/index.js

@@ -171,25 +171,32 @@ function requireDefaultFromModule(modulePath) {
  }
 }

-function scriptObjectToChalk({name, description, script}) {
+function scriptObjectToChalk(options, {name, description, script}) {


You could save a fair amount of logic below by changing this to:

function scriptObjectToChalk(options = {'help-style': 'all'}, {name, description, script}) {

kentcdodds · 2017-10-04T01:37:55Z

src/bin-utils/parser.js

@@ -117,6 +123,7 @@ function parse(rawArgv) {
      log.info(help(psConfig))
      return true
    }
+    const helpStyle = String(psConfig.options['help-style'] || 'all')


Is this || 'all' necessary considering the parser has default: 'all'? I'm pretty sure that psConfig.options['help-style'] should always be defined.

I think I might've put that in because I had been running into some edge cases from the existing tests... but that also may have been because I was mis-understanding the test feedback. I'll give it a shot without it in there 👍

kentcdodds · 2017-10-04T01:39:26Z

README.md

+  --require, -r     Module to preload
+  -h, --help        Show help                                           [boolean]
+  -v, --version     Show version number                                 [boolean]
+  --help-style, -hs Style of help to use


I think that -hs should be -y based on the alias in the parser.

Yup, good catch.

kentcdodds · 2017-10-04T01:39:32Z

README.md

@@ -310,6 +312,25 @@ And you can pass arguments to scripts by putting the scripts in quotes:
 nps "test --cover" check-coverage
 ```

+##### -hs, --help-style


-hs should be -y?

K3TH3R · 2017-10-11T02:30:06Z

@kentcdodds Thank you for taking the time to provide your feedback. I did your requested updates and added myself as a contributor. All of the tests are passing, but I'm still unsure how to fix the code coverage issues?

kentcdodds · 2017-10-12T04:27:24Z

Hi @iamkether! Yeah, code coverage is tricky. I normally require 100% code coverage on my open source projects because they're pretty small and often have many dependents, so it's worth the effort. If you run npm test then you should have a coverage directory. Open coverage/lcov-report/index.html and that'll show you where you're missing coverage (looks like it's the src/bin-utils/parser.js file). Just add a test or two to cover the missing lines and you should be good :)

Let me know if you need help with that :)

K3TH3R · 2017-11-01T05:33:47Z

Hey @kentcdodds, I've taken a look and I could definitely use your help on this. The first line in the code coverage report uncovered is the updated function signature on line 174 of bin-utils/index.js:

function scriptObjectToChalk(
  options = {'help-style': 'all'}, // this line specifically
  {name, description, script},
)

I haven't done much testing previously, so I'm not sure how to test this? The whole point of doing the line like this is so that we set a default and don't really have to test conditions where the value isn't provided, no? How would one even go about testing whether the default assignment has been assigned? Wouldn't I need to peek inside of the function post-call and check the values?

And the second line it's choking on is the following one on line 139-141 of bin-utils/parser.js:

if (helpStyle === 'all') {
  parser.showHelp('log')
}

I expect that I have to fire the showHelp function this is contained within, but I'm not seeing anything on the mockBinUtils object that would allow me to do that? But again, I'm fairly new to testing so there's probably something I'm not understanding quite right here...

kentcdodds · 2017-11-02T17:02:13Z

Thanks @iamkether. So if you cannot reproduce it in tests then the code probably doesn't need to be there at all. Meaning if there's no way for you to have that function called without arguments provided, then we don't need a default argument. Does that make sense? I'm really sorry, but it's very hard for me to find time to help with this :-/

mikecann · 2017-11-02T23:39:54Z

@iamkether ill try to take a look at it for you today, see if I can help..

Ughh or not.. sorry I overcommitted :(

K3TH3R · 2017-11-03T08:20:32Z

@kentcdodds @mikecann I did a little bit of refactoring based on Kent's last message and I'm much closer now. I have 100% coverage for all statements and functions, but it's telling me I'm at 97.67% for branch... not sure what that is? It's still identifying the same line as the problem though.

[edit]
I feel this is probably more due to my lack of experience/knowledge with writing tests more than anything... but it's hard to problem-solve something you don't have much knowledge or experience with.

kentcdodds · 2017-11-03T15:50:33Z

src/bin-utils/parser.js

@@ -129,7 +136,9 @@ function parse(rawArgv) {
      log.info(specificHelpScript(psConfig, specifiedScripts[1]))
      return true
    } else if (commandIsHelp || noScriptSpecifiedAndNoDefault) {
-      parser.showHelp('log')
+      if (helpStyle === 'all' || helpStyle === 'undefined') {


I would change this to: helpStyle === 'all' || !helpStyle

Unless someone explicitly sets helpStyle to the string of 'undefined' then that second branch will never be true.

@kentcdodds okay, so the undefined check was because of the value coming back from the tests. Where I think I might be getting confused and what might be happening is that, I'm expecting the baseOptions to be setting a default everytime the script is run. However, I don't think that's happening when the tests are run, so I'm getting an undefined value for my helpStyle variable.

For example, if I change it back to this:

if (helpStyle === 'all') { parser.showHelp('log') }

and change the helpStyle var to have a default as I had it before:

const helpStyle = String(psConfig.options['help-style'] || 'all')

I will get 100% function/statement/line coverage again, but still 97.67% on the branch stats for that same if (helpStyle === 'all') line. Now, I've read up some more on "branches" and I understand that Jest is calculating multiple different potential branches from this line:

helpStyle === 'all'

helpStyle === 'scripts'

helpStyle === 'basic'

helpStyle === undefined (in the case of aforementioned defaults during tests not being set)

But the way that I've written this is that scripts and basic values are not relevant at this point of the code, because they're analyzed inside of the scriptObjectToChalk() function from index.js. As far as I can tell, it seems to me like I'm going to need to do a bunch of refactoring to move showHelp into index.js and export it to be used in parser.js, because it's not currently exposed as a publically accessible method of the parse object exported from parser.js.

It ultimately makes sense for all of the "help"-related functions/screen output to be within the same file, but I want to check if you see another way before I dive into this because it's going to be a decent-sized refactoring.

kentcdodds · 2017-11-07T16:58:41Z

If you're confident that it works as-is, then you can also just use the comment /* istanbul ignore next */. No worries :)

Thank you so much for working so hard on this. I know it's not been easy and I've not been the best helper.

kentcdodds · 2017-11-07T16:58:55Z

I hope you've learned a lot from the time you've put in :)

K3TH3R · 2017-11-09T02:37:20Z

@kentcdodds it definitely works as is, so I've implemented the istanbul ignore for that if statement. And yes, I've definitely learned some things, thank you :)

kentcdodds · 2017-11-09T02:39:17Z

Looks like the tests aren't working on Travis: https://travis-ci.org/kentcdodds/nps/builds/299433143?utm_source=github_status&utm_medium=notification

K3TH3R · 2017-11-09T04:04:55Z

npm test vs. nps validate 😣

On it.

codecov · 2017-11-09T04:25:29Z

Codecov Report

Merging #164 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #164   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          12     12           
  Lines         376    382    +6     
  Branches       90     92    +2     
=====================================
+ Hits          376    382    +6

Impacted Files	Coverage Δ
src/bin-utils/parser.js	`100% <100%> (ø)`	⬆️
src/bin-utils/index.js	`100% <100%> (ø)`	⬆️

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 fc7e660...250f7c6. Read the comment docs.

K3TH3R · 2017-11-09T04:26:57Z

@kentcdodds realized that I was setting the help-style default in the wrong place. I implemented a small "default" config object and merged it with the config that was getting returned so everything is passing now.

kentcdodds

Thank you so much for working so tirelessly on this! 🎉 Very well done!

kentcdodds · 2017-11-09T05:39:06Z

Thanks so much for your help! I've added you as a collaborator on the project. Please make sure that you review the other/MAINTAINING.md and CONTRIBUTING.md files (specifically the bit about the commit messages and the git hooks) and familiarize yourself with the code of conduct (we're using the contributor covenant). You might also want to watch the repo to be notified when someone files an issue/PR. Please continue to make PRs as you feel the need (you can make your branches directly on the repo rather than your fork if you want). Thanks! And welcome to the team :)

K3TH3R · 2017-11-09T05:59:08Z

Sounds good @kentcdodds, and thank you for making me part of the team :)

K3TH3R added 2 commits October 2, 2017 11:25

WIP: Help Style configuration to eliminate excessively long help output

2ab2b33

WIP: trying to solve these testing issues

9b078d2

K3TH3R added 2 commits October 3, 2017 14:00

WIP: tests passing but missing cli flags

1028c4f

WIP: added tests for CLI flags

e69b9b2

K3TH3R and others added 4 commits October 3, 2017 14:51

WIP: change help-style alias to 'y' to maintain single char flag/dash…

59c7fbb

… pattern

docs: Utils documentation fix (#163)

1a71f44

* Update concurrently => concurrent in examples * Add myself as a contributer * Add myself as a contributer properly, this time :)

wIP: started on readme docs

09eaf56

Merge branch 'master' into help-style

d1acec5

kentcdodds reviewed Oct 4, 2017

View reviewed changes

K3TH3R added 2 commits October 11, 2017 09:24

WIP: updated a couple files based on feedback in the pull request

b4c0583

WIP: added myself as a contributor

8166d50

K3TH3R added 2 commits November 3, 2017 15:07

WIP: moved help-style defaults to function calls

6ddccea

WIP: added check for undefined to helpstyle, almost 100% code coverage

508648d

kentcdodds reviewed Nov 3, 2017

View reviewed changes

feat: implemented configuration options for Help Style

8dfb64e

fix: making sure tests/validations run properly

250f7c6

kentcdodds approved these changes Nov 9, 2017

View reviewed changes

kentcdodds merged commit 9338d72 into sezna:master Nov 9, 2017

K3TH3R deleted the help-style branch November 9, 2017 05:53

K3TH3R mentioned this pull request Nov 9, 2017

Add option to display only name and description for help command #119

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Help style #164

[WIP] Help style #164

K3TH3R commented Oct 2, 2017 •

edited

kentcdodds commented Oct 2, 2017

K3TH3R commented Oct 3, 2017

kentcdodds left a comment

kentcdodds Oct 4, 2017

kentcdodds Oct 4, 2017

K3TH3R Oct 11, 2017

kentcdodds Oct 4, 2017

K3TH3R Oct 11, 2017

kentcdodds Oct 4, 2017

K3TH3R commented Oct 11, 2017

kentcdodds commented Oct 12, 2017

K3TH3R commented Nov 1, 2017

kentcdodds commented Nov 2, 2017

mikecann commented Nov 2, 2017 •

edited

K3TH3R commented Nov 3, 2017 •

edited

kentcdodds Nov 3, 2017

K3TH3R Nov 7, 2017

kentcdodds commented Nov 7, 2017

kentcdodds commented Nov 7, 2017

K3TH3R commented Nov 9, 2017

kentcdodds commented Nov 9, 2017

K3TH3R commented Nov 9, 2017

codecov bot commented Nov 9, 2017 •

edited

K3TH3R commented Nov 9, 2017

kentcdodds left a comment

kentcdodds commented Nov 9, 2017

K3TH3R commented Nov 9, 2017

[WIP] Help style #164

[WIP] Help style #164

Conversation

K3TH3R commented Oct 2, 2017 • edited

kentcdodds commented Oct 2, 2017

K3TH3R commented Oct 3, 2017

kentcdodds left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

K3TH3R commented Oct 11, 2017

kentcdodds commented Oct 12, 2017

K3TH3R commented Nov 1, 2017

kentcdodds commented Nov 2, 2017

mikecann commented Nov 2, 2017 • edited

K3TH3R commented Nov 3, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kentcdodds commented Nov 7, 2017

kentcdodds commented Nov 7, 2017

K3TH3R commented Nov 9, 2017

kentcdodds commented Nov 9, 2017

K3TH3R commented Nov 9, 2017

codecov bot commented Nov 9, 2017 • edited

Codecov Report

K3TH3R commented Nov 9, 2017

kentcdodds left a comment

Choose a reason for hiding this comment

kentcdodds commented Nov 9, 2017

K3TH3R commented Nov 9, 2017

K3TH3R commented Oct 2, 2017 •

edited

mikecann commented Nov 2, 2017 •

edited

K3TH3R commented Nov 3, 2017 •

edited

codecov bot commented Nov 9, 2017 •

edited