New commands: sort(), head(), and tail() by nfischer · Pull Request #379 · shelljs/shelljs

nfischer · 2016-02-29T19:28:15Z

These are initial versions of the head() and sort() commands. Not ready to merge. Feel free to take a look and give feedback on what I have implemented so far.

I haven't written unit tests yet, but I will do so soon.

The reason these are together in one PR is that I wanted to be able to hack out a translation of an adaptation of the famous Donald Knuth shell problem:

#!/bin/bash
cat input.txt |
tr -cs A-Za-z '\n' |
tr A-Z a-z |
sort |
uniq -c |
sort -rn |
head -n $1

Will now (correctly) translate to:

#!/usr/bin/env node
require('shelljs/global');
config.silent = true;
echo(cat('input.txt')
  .exec('tr -cs A-Za-z \'\\n\'')
  .exec('tr A-Z a-z')
  .sort() // sort works!
  .exec('uniq -c')
  .sort('-rn') // it even takes options!
  .head('-n', process.argv[2])); // and head() should take options of the style `head({'-n', 5}, 'file.txt')`

Also, if someone wants tail(), chime in and I can write that out as well.

nfischer · 2016-03-01T01:19:25Z

I just added in the test cases, so this is ready for review. If anyone from the community wants to chime in for features, please do.

The semantics are slightly different than unix's sort: capitalized words are sorted before non-capitalized words in this function, but unix sort doesn't prefer them. So we sort things like:

Foo
bar

While unix sorts things like:

bar
Foo

This difference could be avoided, but I'm not sure if it really matters so much.

nfischer · 2016-03-01T01:22:43Z

@ariporad If you get the chance, this should be ready for review. I see that it's failing on v0.10, but I think this will pass on the other versions. I can investigate v0.10 if we think it's a priority.

Feel free to review but not merge, since there's no huge rush to get these features. We can always use more input from the community. On the other hand, if we merge now, we can always go back and add features before the next release.

ariporad · 2016-03-01T01:23:51Z

README.md

+Examples:
+
+```javascript
+var str = head({'-n, 1}, 'file*.txt');


You're missing a quote her.

Should be fixed now (good catch)

nfischer · 2016-03-01T01:30:00Z

Looks like there's iojs compatibility issues. I'll try to fix that today and see if I can get v0.10 working as well. I may also try to get unix semantics (I think I know how to do that).

ariporad · 2016-03-01T01:30:18Z

A few nits, otherwise looks good.

nfischer · 2016-03-01T02:04:09Z

Ok, I think I fixed the inconsistency. Also, this is now compatible with how unix sort works, which is a plus. Should be good to merge now.

nfischer · 2016-03-04T17:04:15Z

Let's hold off to merge this. I think this should be refactored to read input in via buffers (vs. reading it all at once), when possible. This is to account for the case where a file is infinite in size (or maybe just super large) and we print out the first 10 lines, keeping the operation constant time instead of hanging indefinitely.

I should have time to refactor this today. I can also add in tail() to the mix in another PR, for the sake of completeness (tail prints the last 10 lines, as opposed to head which prints the first 10 lines).

ariporad · 2016-03-06T00:00:28Z

SGTM!

nfischer · 2016-03-20T20:58:50Z

@ariporad I refactored this into 3 commits. tail() is a new command. This also modifies head() so that it only reads as much of the file as necessary (so it works for files that are infinite in length, like if you run yes > file.txt).

Leave any comments/concerns and I can address when I get back.

nfischer · 2016-03-26T22:10:27Z

@ariporad This should be ready for review

ariporad · 2016-03-27T03:20:54Z

src/sort.js

+
+//@
+//@ ### sort([options,] file [, file ...])
+//@ ### sed([options,] file_array)


Why is this also called sed? I thought sed was find/replace?

Whoops! That was a mistake. I'll fix that

ariporad · 2016-03-27T03:22:01Z

@nfischer: One comment, also can you either squash your final commit or make it comply with the commit guidelines? Thanks!

nfischer · 2016-03-28T05:32:13Z

@ariporad Rebased this and fixed the issues

nfischer · 2016-03-29T00:34:39Z

@ariporad Let me know once you've taken a look at this. I'll address any issues and rebase. If there are no issues, feel free to give the thumbs up and I'll rebase and merge.

ariporad · 2016-04-01T04:03:46Z

LGTM!

nfischer · 2016-04-01T04:05:06Z

Ok great! I'll rebase this and merge

nfischer · 2016-04-01T04:22:03Z

LGTM (because of rebase)

facundoolano · 2016-04-20T18:55:31Z

any chance of having this published to npm?

nfischer · 2016-04-20T20:19:05Z

Hopefully soon. We had some last minute concerns raised, but it should be resolved soon

nfischer added feature medium priority bash compat Compatibility issues with bash or POSIX behavior labels Feb 29, 2016

nfischer self-assigned this Feb 29, 2016

nfischer added this to the v0.7.0 milestone Feb 29, 2016

ariporad reviewed Mar 1, 2016
View reviewed changes

nfischer force-pushed the feat-head-sort-commands branch 3 times, most recently from 373b9c1 to 3140e60 Compare March 1, 2016 02:03

nfischer changed the title ~~Head sort commands~~ Adding the head() and sort() commands Mar 1, 2016

nfischer changed the title ~~Adding the head() and sort() commands~~ New commands: head() and sort() Mar 1, 2016

nfischer force-pushed the feat-head-sort-commands branch from 3140e60 to c66c947 Compare March 20, 2016 20:57

ariporad force-pushed the feat-head-sort-commands branch from c66c947 to eceea45 Compare March 20, 2016 21:30

nfischer changed the title ~~New commands: head() and sort()~~ New commands: sort(), head(), and tail() Mar 24, 2016

nfischer force-pushed the feat-head-sort-commands branch 2 times, most recently from b9d032f to 7658a8b Compare March 26, 2016 22:09

ariporad reviewed Mar 27, 2016
View reviewed changes

nfischer force-pushed the feat-head-sort-commands branch from 7658a8b to 101b25e Compare March 28, 2016 05:29

nfischer force-pushed the feat-head-sort-commands branch from 101b25e to ef3a740 Compare March 28, 2016 05:31

nfischer added 3 commits March 31, 2016 21:05

feat(command): new command: sort()

f6952cb

feat(command): new command: head()

a1d5f6a

feat(command): new command: tail()

60d6301

nfischer force-pushed the feat-head-sort-commands branch from ef3a740 to 60d6301 Compare April 1, 2016 04:06

nfischer merged commit 2984b40 into master Apr 1, 2016

nfischer deleted the feat-head-sort-commands branch April 1, 2016 04:22

Conversation

nfischer commented Feb 29, 2016

Uh oh!

nfischer commented Mar 1, 2016

Uh oh!

nfischer commented Mar 1, 2016

Uh oh!

ariporad Mar 1, 2016

Choose a reason for hiding this comment

Uh oh!

nfischer Mar 1, 2016

Choose a reason for hiding this comment

Uh oh!

nfischer Mar 1, 2016

Choose a reason for hiding this comment

Uh oh!

nfischer commented Mar 1, 2016

Uh oh!

ariporad commented Mar 1, 2016

Uh oh!

nfischer commented Mar 1, 2016

Uh oh!

nfischer commented Mar 4, 2016

Uh oh!

ariporad commented Mar 6, 2016

Uh oh!

nfischer commented Mar 20, 2016

Uh oh!

nfischer commented Mar 26, 2016

Uh oh!

ariporad Mar 27, 2016

Choose a reason for hiding this comment

Uh oh!

nfischer Mar 27, 2016

Choose a reason for hiding this comment

Uh oh!

ariporad commented Mar 27, 2016

Uh oh!

nfischer commented Mar 28, 2016

Uh oh!

nfischer commented Mar 29, 2016

Uh oh!

ariporad commented Apr 1, 2016

Uh oh!

nfischer commented Apr 1, 2016

Uh oh!

nfischer commented Apr 1, 2016

Uh oh!

facundoolano commented Apr 20, 2016

Uh oh!

nfischer commented Apr 20, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants