Add "-n" option to echo #590

freitagbr · Nov 28, 2016

Fixes #559

nfischer · Nov 28, 2016

ava-test/echo.js

+});
+
+test.cb('-ne option', t => {
+  const script = "require('../global.js'); echo('-ne', 'message');";


Can we also test echo -en 'foo\n' to make sure that -e still works?

Sure, I can add a test for that.

nfischer · Nov 28, 2016

src/echo.js

+    output = format.apply(null, messages) + '\n';
  }

-  console.log.apply(console, messages);


Let's only use process.stdout.write for when -n is passed. Calling process.exit immediately after process.stdout.write has some issues.

I don't think it matters, because console.log is just a wrapper around process.stdout.write. See this.

For reference, it has been that way since at least v0.7.4.

Hmmm ok, let's use this approach then.

nfischer · Nov 28, 2016

src/echo.js

+  var option = ['-e', '-n', '-ne', '-en'].indexOf(messages[0]);
+  var output;
+
+  if (option >= 0) {


It might be easier to just check:

is the first argument a real option?

do the options contain -n?

And just switch the behavior on that condition. That way we can keep using console.log by default, which has some nicer behavior (see my comment below).

How do we check if the first argument is a real option? Would using the cmdOptions attribute in common.register solve this?

I think we can use common.parseOptions to parse the first argument. If we catch the option not recognized error, then just print that string.

Yeah, but it won't throw the error as an exception, it'll return early from the echo function. You can hack around it by setting config.fatal to have it throw an exception. A clearer way is to add an option to common.error to throw an exception instead of exiting (regardless of config.fatal).

The approach you have here works if we only have two options, but it gets cumbersome if we ever support -E (which we may want to support in the future).

Ok, I've added a function to echo.js that parses options just for echo. This means adding -E later on will be easier.

nfischer · Nov 29, 2016

src/echo.js

+  return output;
 }
+
+function parseEchoOptions(opts) {


Actually, let's use common.parseOptions(). It actually does throw an exception upon error regardless of config.fatal. It behaves exactly how we want, and it'll be much cleaner to reuse the code.

parseOptions will still log to stdout when it doesn't recognize an option:

> try { ... common.parseOptions('-b', {a: 'test'}) ... } catch (_) { ... console.log('whoops') ... } shell.js: option not recognized: b whoops

This is not good if someone tries to echo a string that happens to start with '-'.

Ah, I see the issue. I think it may be better to either:

hack around this by temporarily setting config.silent = true (document what we're doing in comments)

add a way to silence parseOptions (exception only, no stdio)

I think the approach we take depends on if we want to expose this feature in parseOptions in the plugin API.

@freitagbr What do you think?

I would prefer an extra silence argument or option for parseOptions. Plugins could probably make use of the option, too.

freitagbr · Dec 7, 2016

@nfischer I refactored echo to use common.parseOptions, can you take a look when you get a chance?

nfischer · Dec 8, 2016

@freitagbr please rebase off dev

freitagbr · Dec 8, 2016

Done.

nfischer · Dec 8, 2016

src/echo.js

-    // ignore -e
+  if (options) {
+    // first argument was options string,
+    // so do not print it


Join these lines?

nfischer · Dec 8, 2016

src/echo.js

+  // If the first argument starts with '-',
+  // parse it as options string.
+  // If parseOptions throws,
+  // it wasn't an options string.


Join these lines?

nfischer · Dec 8, 2016

src/echo.js

+      output += '\n';
+    }
+  } else {
+    output = format.apply(null, messages) + '\n';


We're appending \n in this branch, but we're also appending it above. Can we flatten-out this logic? Something like:

try { options = common.parseOptions(messages[0], { ... }, { silent: true }); messages.shift(); } catch (e) { options = {}; } output = format.apply(null, messages); if (!options.no_newline) { output += '\n'; } process.stdout.write(output);

I think we still need to check if the first argument starts with '-', because we don't know if it needs to be printed or not otherwise.

Ok, sure. parseOptions has useless behavior if you pass it a string that doesn't start with -.

nfischer

Approval. See #614 in case you want to fix that first, otherwise FFTM

freitagbr · Dec 9, 2016

I'll wait for the pull request I submitted (#615) to be merged before moving on here.

freitagbr · Dec 10, 2016

There's an issue here. When passing a string starting with -, but containing unrecognized options, stderr on the return object is set:

> echo('-hi')
-hi
{ [String: '-hi\n']
  stdout: '-hi\n',
  stderr: 'echo: option not recognized: h',
  code: 1,
  cat: [Function: bound ],
  head: [Function: bound ],
  tail: [Function: bound ],
  to: [Function: bound ],
  toEnd: [Function: bound ],
  sed: [Function: bound ],
  sort: [Function: bound ],
  uniq: [Function: bound ],
  grep: [Function: bound ],
  exec: [Function: bound ] }

This is obviously not how echo behaves in the shell.

nfischer · Dec 10, 2016

@freitagbr can you manually clear out common.state.error? That should do the trick.

I doubt there's a great way to test it, but maybe you could add a test like:

const ret = shell.echo('-n', ''); // echo nothing
t.falsy(ret.stderr);
t.is(ret.stdout, '');
t.is(ret.code, 0);

This case wouldn't add stderr anyway, but it'll save us from huge future regressions while also not polluting the output.

needs change

nfischer

Needs a fix for .stderr

freitagbr · Dec 11, 2016

@nfischer I added the fix, though there is already a test case that checks that stderr === ''

nfischer · Dec 12, 2016

@freitagbr it needs to check for ret.stderr. The .stderr isn't ever being outputted (that's what the runScript checks), but it was being returned as a property (so we need to check function return value).

freitagbr · Dec 14, 2016

Ok, test added. PTAL

nfischer · Dec 14, 2016

ava-test/echo.js

 });
+
+test('stderr with unrecognized options is null', t => {
+  const ret = shell.echo('-asdf');


@freitagbr will this output to the console? If so, let's at least add a TODO to mute console output

Yes it does. I suppose we can fix this along with mocking process.stdout like you suggested for #622.

nfischer · Dec 14, 2016

ava-test/echo.js

  });
 });
+
+test('stderr with unrecognized options is null', t => {


s/null/empty/ or s/null/falsy/

We may want to switch stderr from null to the empty string in a future release.

freitagbr · Dec 16, 2016

@nfischer PTAL

freitagbr changed the base branch from master to dev November 28, 2016 00:35

nfischer requested changes Nov 28, 2016

View reviewed changes

freitagbr assigned nfischer Nov 28, 2016

nfischer requested changes Nov 29, 2016

View reviewed changes

nfischer mentioned this pull request Nov 30, 2016

parseOptions: allow a way to keep errors silent (exception only) #591

Closed

freitagbr force-pushed the echo-stdout branch from 4f4b422 to 74242aa Compare December 7, 2016 09:11

freitagbr force-pushed the echo-stdout branch from 74242aa to 09d8643 Compare December 8, 2016 04:17

nfischer force-pushed the dev branch from 23ef7eb to f996bd1 Compare December 8, 2016 04:51

freitagbr force-pushed the echo-stdout branch from 09d8643 to 11629fb Compare December 8, 2016 05:21

nfischer requested changes Dec 8, 2016

View reviewed changes

nfischer mentioned this pull request Dec 9, 2016

parseOptions should throw an error if the option string doesn't start with '-' #614

Closed

nfischer previously approved these changes Dec 9, 2016

View reviewed changes

freitagbr added 10 commits December 9, 2016 18:09

Add -n option to echo

357cf3e

Fix null argument issue

781bf6a

Add -n tests

ac3955b

Add documentation

ddedf7d

Add -en escaped character test

4cba92f

Add function to parse options for echo

ffb58e4

Use parseOptions to parse echo options

5e215af

Simplify control flow

66fb85d

parseOptions throws now

ee3fa15

Allow null to be echoed

7566ac4

nfischer requested changes Dec 10, 2016

View reviewed changes

Prevent echo stderr on unrecognized option

e018604

freitagbr force-pushed the echo-stdout branch from 609d7c8 to e018604 Compare December 11, 2016 01:10

Add test to check stderr of returned value

08fe067

Use consistent variable name

0dceb82

nfischer requested changes Dec 14, 2016

View reviewed changes

Change test message, leave TODO about console output

37ea827

freitagbr force-pushed the echo-stdout branch from ec4810e to 37ea827 Compare December 15, 2016 04:43

nfischer approved these changes Dec 19, 2016

View reviewed changes

freitagbr merged commit 53743f4 into dev Dec 19, 2016

freitagbr deleted the echo-stdout branch December 22, 2016 02:18

nfischer mentioned this pull request Dec 27, 2016

feature: echo -n #559

Closed

nfischer restored the echo-stdout branch February 28, 2017 06:05

nfischer mentioned this pull request Feb 28, 2017

Echo stdout #677

Merged

nfischer mentioned this pull request Jun 7, 2017

Merge dev into master #731

Merged

snyk-bot mentioned this pull request Sep 5, 2019

[Snyk] Upgrade shelljs from 0.7.0 to 0.8.3 MailOnline/mol-cz-lerna-changelog#1

Open

snyk-bot mentioned this pull request Sep 19, 2019

[Snyk] Upgrade shelljs from 0.3.0 to 0.8.3 ali8889/byteball#2

Open

snyk-bot mentioned this pull request Nov 29, 2019

[Snyk] Upgrade shelljs from 0.3.0 to 0.8.3 sharksandcupcakes/html-inspector#1

Open

snyk-bot mentioned this pull request Mar 8, 2020

[Snyk] Upgrade shelljs from 0.3.0 to 0.8.3 michelvankessel/colored-coins-copay-wallet#1

Open

Search code, repositories, users, issues, pull requests...

Add "-n" option to echo #590

Add "-n" option to echo #590

Conversation

freitagbr commented Nov 28, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

freitagbr commented Dec 7, 2016

Uh oh!

nfischer commented Dec 8, 2016

Uh oh!

freitagbr commented Dec 8, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nfischer left a comment

Choose a reason for hiding this comment

Uh oh!

freitagbr commented Dec 9, 2016

Uh oh!

freitagbr commented Dec 10, 2016

Uh oh!

nfischer commented Dec 10, 2016

Uh oh!

nfischer left a comment

Choose a reason for hiding this comment

Uh oh!

freitagbr commented Dec 11, 2016

Uh oh!

nfischer commented Dec 12, 2016

Uh oh!

freitagbr commented Dec 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

freitagbr commented Dec 16, 2016

Uh oh!

Reviewers

Assignees