refactor(cli): display which script is executed #136

Miklet · 2017-04-23T19:05:52Z

Added an additional information displaying which script is executed.

What: Added information which script is executed.

Why: Because of #126

How: Just added an additional chalk call and changed string concatenation for template literal.

codecov-io · 2017-04-23T19:07:29Z

Codecov Report

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

@@          Coverage Diff          @@
##           master   #136   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          10     10           
  Lines         302    308    +6     
=====================================
+ Hits          302    308    +6

Impacted Files	Coverage Δ
src/bin-utils/initialize/index.js	`100% <100%> (ø)`	⬆️
src/get-script-to-run.js	`100% <100%> (ø)`	⬆️
src/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 061b5c3...274ebd3. Read the comment docs.

kentcdodds

Could we get a test to see what it looks like when I run a script foo.bar.baz (for example), but use the prefix fo.b.ba (for example)?

kentcdodds · 2017-04-23T21:30:32Z

Thanks for working on this!

Miklet · 2017-04-24T07:07:45Z

It looks like that:

kentcdodds · 2017-04-24T13:47:34Z

Thanks for the screenshot! The use case is: I've entered a prefix version of the script, so I know what I entered. I want to see the full name of the script to validate that the script that's running is the one I'm thinking. Does that make sense?

Miklet · 2017-04-24T14:07:00Z

Yes, it does make sense if I understood it properly 😄 So instead of:
nps is executing fo.b.ba: echo "foo.bar.baz"
you expect:
nps is executing foo.bar.baz: echo "foo.bar.baz". Am I right?

kentcdodds · 2017-04-24T14:10:14Z

Correct 😀

Miklet · 2017-04-24T18:52:38Z

It seemed to be pretty easy issue but it turned out to be a little bit more complicated. I thought that there is somewhere some lonely variable with resolved script name but I was wrong 😄 I am thinking now about some function which will try to find matching key by a value by traversing recurisvely script's object. I will encapsulate it to separate module and it will be easy to test.

Miklet · 2017-04-25T16:56:29Z

Added script's name resolving. Let me know if you like it and I will squash commits into one before merge.

kentcdodds

Thanks! More questions :)

kentcdodds · 2017-04-25T17:50:41Z

src/resolve-script-name.js

+
+export default resolveScriptName
+
+function resolveScriptName(scriptToFind, scriptsConfig) {


Rather than scriptToFind, could this be passed the prefix? (like fo.baz.f could find foo.baz.foobar)

Not really, because this function search for a property by given value (scriptToFind). Function which resolves what you mentioned is already implemented and it is a prefix-matcher package.

Sorry, what I meant to say was, that's what I'd rather it do 😄

I understand, but I actually think the way it is, it is better. Because I don't see a way to get resolved script name by it's prefix in other way than reimplementing prefix-matcher 😄 I may be wrong, correct me if you see other solution.

Couldn't we extend getScriptToRun to get the script name in addition to the script command?

Couldn't we extend getScriptToRun to get the script name in addition to the script command?

Maybe. Without looking at the code I'm unsure. If so, that'd be great.

reimplementing prefix-matcher

Maybe we could contribute back and expose a method to do what we need :)

Note that I have merge (and maybe release?) rights on that repo.

The best solution in my opinion would to be contribute to prefix-matcher, implement returning property (whole chain, not only the deepest) and then we could extend getScriptToRun with it.

That sounds good to me 👍

Miklet · 2017-05-26T22:01:07Z

Hi, I bumped up a version of prefix-matches and used the newest version to resolve the issue.

kentcdodds

This is looking great! I'm really excited to get this. Thanks for all your work on this!

kentcdodds · 2017-05-26T22:14:09Z

cli-test/__snapshots__/cli.test.js.snap

@@ -12,7 +12,7 @@ lint.sub.hiddenThing
 exports[`test with --require 1`] = `
 Object {
  "stderr": "",
-  "stdout": "nps executing: echo \"log\"
+  "stdout": "nps is executing log: echo \"log\"


Could this be:

nps is executing `log`: echo "log"

kentcdodds · 2017-05-26T22:14:28Z

cli-test/cli.test.js

@@ -17,6 +17,9 @@ test('with --require', () =>
 test('with --get-yargs-completions', () =>
  snapshot('--config ./package-scripts.js --get-yargs-completions li'))

+test('with prefix', () =>


kentcdodds · 2017-05-26T22:16:42Z

src/get-script-to-run.js

+      scriptToRun = resolveScriptObjectToString(scriptToRun)
+    }
+    return {
+      resolvedPrefix,


Could we call this scriptName instead? Because it's not the prefix, it's actually what the prefix was resolved to, which should be the full script name.

Yea, that would actually be much better name.

Updated prefix-matches verion and added script name of executed script

Miklet · 2017-05-27T13:34:41Z

Updated with your comments.

kentcdodds

Perfect!

kentcdodds · 2017-05-27T13:36:27Z

Thank you for this!

Miklet requested a review from kentcdodds April 23, 2017 19:05

kentcdodds reviewed Apr 23, 2017

View reviewed changes

kentcdodds reviewed Apr 25, 2017

View reviewed changes

Miklet force-pushed the pr/executing_text branch 2 times, most recently from a04d526 to 06558c4 Compare May 26, 2017 21:49

kentcdodds reviewed May 26, 2017

View reviewed changes

refactor(cli): display name of executing script (#126)

274ebd3

Updated prefix-matches verion and added script name of executed script

Miklet force-pushed the pr/executing_text branch from 06558c4 to 274ebd3 Compare May 27, 2017 12:53

kentcdodds approved these changes May 27, 2017

View reviewed changes

kentcdodds merged commit 76bee57 into master May 27, 2017

kentcdodds deleted the pr/executing_text branch May 27, 2017 13:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(cli): display which script is executed #136

refactor(cli): display which script is executed #136

Miklet commented Apr 23, 2017

codecov-io commented Apr 23, 2017 •

edited

kentcdodds left a comment

kentcdodds commented Apr 23, 2017

Miklet commented Apr 24, 2017

kentcdodds commented Apr 24, 2017

Miklet commented Apr 24, 2017

kentcdodds commented Apr 24, 2017

Miklet commented Apr 24, 2017

Miklet commented Apr 25, 2017

kentcdodds left a comment

kentcdodds Apr 25, 2017

Miklet Apr 25, 2017 •

edited

kentcdodds Apr 25, 2017

Miklet Apr 25, 2017 •

edited

tleunen Apr 25, 2017

kentcdodds Apr 26, 2017

kentcdodds Apr 26, 2017

Miklet Apr 26, 2017

kentcdodds Apr 26, 2017

Miklet commented May 26, 2017

kentcdodds left a comment

kentcdodds May 26, 2017

kentcdodds May 26, 2017

kentcdodds May 26, 2017

Miklet May 27, 2017

Miklet commented May 27, 2017

kentcdodds left a comment

kentcdodds commented May 27, 2017


		export default resolveScriptName

		function resolveScriptName(scriptToFind, scriptsConfig) {

refactor(cli): display which script is executed #136

refactor(cli): display which script is executed #136

Conversation

Miklet commented Apr 23, 2017

codecov-io commented Apr 23, 2017 • edited

Codecov Report

kentcdodds left a comment

Choose a reason for hiding this comment

kentcdodds commented Apr 23, 2017

Miklet commented Apr 24, 2017

kentcdodds commented Apr 24, 2017

Miklet commented Apr 24, 2017

kentcdodds commented Apr 24, 2017

Miklet commented Apr 24, 2017

Miklet commented Apr 25, 2017

kentcdodds left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Miklet Apr 25, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Miklet Apr 25, 2017 • edited

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

Miklet commented May 26, 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

Miklet commented May 27, 2017

kentcdodds left a comment

Choose a reason for hiding this comment

kentcdodds commented May 27, 2017

codecov-io commented Apr 23, 2017 •

edited

Miklet Apr 25, 2017 •

edited

Miklet Apr 25, 2017 •

edited