Skip to content

feat(ls): add -l option#324

Merged
nfischer merged 1 commit intomasterfrom
ls-l-option
Jan 31, 2016
Merged

feat(ls): add -l option#324
nfischer merged 1 commit intomasterfrom
ls-l-option

Conversation

@nfischer
Copy link
Copy Markdown
Member

The -l option will now cause ls() to return an object containing file stats.
These objects will also have a toString() method that formats it into something
analogous to ls -l's output format.

This is useful in the case of ls('-l', 'dir/').join('\n'), which will now have formatting similar to unix's ls -l.

The mechanics for -l are based off #137. This resolves feature request #323.

@nfischer
Copy link
Copy Markdown
Member Author

@gvn @ariporad this should be ready for review.

I believe Windows does not support uid and gid for fs.statSync() so I'm merely ignoring these values in the tests.

Here's a comparison of output from ls -l and ls('-l', ...):

$ ls -l tmp/54.js
-rw-rw-r-- 1 nate nate 54 Jan 29 15:58 tmp/54.js

> ls('-l', 'tmp/54.js').toString();
33204 1 1000 1000 54 Fri Jan 29 2016 15:58:26 GMT-0800 (PST) tmp/54.js

The dates are javascript dates. The uid and gid are ID numbers, not username/groupname (this npm module uses C code to translate ID to name). The modes are in whatever representation fs.stat() returns them. Although the appearance isn't identical, I think using the JS types will benefit users more.

src/ls.js Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'd suggest making this value just the filename instead of the entire path, and then adding another property called path with the full path. Seems a little clearer that way IMO.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I see the benefit, but it could be slightly trickier that way. ls -l returns output with the relative path to the file, so that's what I'm opting for with this. If users need the specific name, they can use path.basename() to get it. If they need the full path, I think path.resolve() would do it.

I could use these functions to set the internal properties, but it's harder to use the full path and basename to get the relative path for the toString() method.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Ah, I see. In that case path or relativePath would be more semantic than name, but only a by a little bit, so close enough! 😉

@gvn
Copy link
Copy Markdown

gvn commented Jan 30, 2016

I had a couple thoughts, but otherwise this looks great! I tested on OS X w Node v5.4.1

Thanks for jumping on this so quickly. 👍

PS: Great idea with the toString method!

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I forgot to add docs for the -d option, so I'm adding them now.

@ariporad
Copy link
Copy Markdown
Contributor

@nfischer: One small comment, then looks good.

@ariporad ariporad added this to the v0.6.0 milestone Jan 31, 2016
The `-l` option will now cause `ls()` to return an object containing file stats.
These objects will also have a toString() method that formats it into something
analogous to `ls -l`'s output format.
@ariporad
Copy link
Copy Markdown
Contributor

LGTM!

nfischer added a commit that referenced this pull request Jan 31, 2016
@nfischer nfischer merged commit d8466a1 into master Jan 31, 2016
@gvn
Copy link
Copy Markdown

gvn commented Jan 31, 2016

This is working great. Thank you @nfischer !

@ariporad ariporad deleted the ls-l-option branch January 31, 2016 22:52
@nfischer
Copy link
Copy Markdown
Member Author

👍 My pleasure

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants