Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update doc comments and regenerate README.md. #825

Merged
merged 5 commits into from Feb 20, 2018

Conversation

Zearin
Copy link
Contributor

@Zearin Zearin commented Feb 9, 2018

No description provided.

@Zearin
Copy link
Contributor Author

Zearin commented Feb 9, 2018

Follow-up to #824. :)

Copy link
Member

@nfischer nfischer left a comment

Choose a reason for hiding this comment

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

Terms used as improper nouns don't need backticks (e.g., "This command operates over each file" instead of "This command operates over each file"). I commented on some of these, but not every instance.

Everything else looks mostly good, I can give another pass later.

README.md Outdated
@@ -151,7 +151,7 @@ var str = cat('file1', 'file2');
var str = cat(['file1', 'file2']); // same as above
```

Returns a string containing the given file, or a concatenated string
Returns a string containing the given `file`, or a concatenated string
Copy link
Member

Choose a reason for hiding this comment

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

No backticks on "file" (it's a noun, not a variable)

README.md Outdated
Available options:

+ `-v`: output a diagnostic for every file processed
+ `-c`: like verbose but report only when a change is made
+ `-v`: output a diagnostic for every `file` processed
Copy link
Member

Choose a reason for hiding this comment

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

Same as above

README.md Outdated

### popd([options,] ['-N' | '+N'])
Copy link
Member

Choose a reason for hiding this comment

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

Did we change the blank lines around this line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There was 1 less blank line above this than the other headings, so I made it match.

README.md Outdated
@@ -213,15 +212,14 @@ Copies files.


### pushd([options,] [dir | '-N' | '+N'])

Copy link
Member

Choose a reason for hiding this comment

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

Missing blank line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed there were sometimes blank lines after a heading, and sometimes not. I looked at all of them and it seemed like the more common case was no blank line, so I made it match.

Copy link
Member

Choose a reason for hiding this comment

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

Blank line is probably more appropriate, actually. Markdown is weird, but "regular line followed immediately by a bullet" doesn't work, while "regular line followed by a blank line and then by a bullet" does work.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, we don't even have a bullet here. Point still stands though:

regular line
regular line

render as: "regular line regular line"

while:

## header line
regular line

renders as:

header line

regular line


I usually add a blank line in-between for consistency with the regular-line case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright! I’ll add a blank line after each header.

README.md Outdated

Not seeing the behavior you want? `exec()` runs everything through `sh`
by default (or `cmd.exe` on Windows), which differs from `bash`. If you
need bash-specific behavior, try out the `{shell: 'path/to/bash'}` option.

**Note:** For long-lived processes, it's best to run `exec()` asynchronously as
**Note:** For long-lived processes, it's best to run `exec()` asynchronously, as
Copy link
Member

Choose a reason for hiding this comment

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

Actually, you can just remove this. The CPU concerns are no longer relevant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do!

README.md Outdated
file that match the given `regex_filter`.


### head([{'-n': \<num\>},] file [, file ...])
### head([{'-n': \<num\>},] file_array)
Available options:

+ `-n <num>`: Show the first `<num>` lines of the files
+ `-n <num>`: Show the first `<num>` lines of the `file`s
Copy link
Member

Choose a reason for hiding this comment

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

No backticks

README.md Outdated
@@ -384,7 +382,7 @@ var str = head('file1', 'file2');
var str = head(['file1', 'file2']); // same as above
```

Read the start of a file.
Read the start of a `file`.
Copy link
Member

Choose a reason for hiding this comment

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

No backticks

README.md Outdated


### mkdir([options,] dir [, dir ...])
### mkdir([options,] dir_array)
Available options:

+ `-p`: full path (will create intermediate dirs if necessary)
+ `-p`: full path (will create intermediate `dir`s, if necessary)
Copy link
Member

Choose a reason for hiding this comment

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

Just change this to "directories" (no backticks)

README.md Outdated
This is a partial implementation of *[touch(1)](http://linux.die.net/man/1/touch)*.
Update the access and modification times of each `FILE` to the current time.
A `FILE` argument that does not exist is created empty, unless `-c` is supplied.
This is a partial implementation of *[`touch(1)`](http://linux.die.net/man/1/touch)*.
Copy link
Member

Choose a reason for hiding this comment

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

Let's remove italics

README.md Outdated

**Note**: do not rely on the
**Note**: Do not rely on the
Copy link
Member

Choose a reason for hiding this comment

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

No capital

@nfischer
Copy link
Member

nfischer commented Feb 9, 2018

Also, it looks like you need to re-run npm run gendocs (Travis uses this to catch for inconsistencies between the README and the doc-comments).

@codecov-io
Copy link

codecov-io commented Feb 10, 2018

Codecov Report

Merging #825 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #825   +/-   ##
=======================================
  Coverage   95.49%   95.49%           
=======================================
  Files          34       34           
  Lines        1266     1266           
=======================================
  Hits         1209     1209           
  Misses         57       57
Impacted Files Coverage Δ
src/cp.js 86.29% <ø> (ø) ⬆️
src/toEnd.js 100% <ø> (ø) ⬆️
src/common.js 98.39% <ø> (ø) ⬆️
src/test.js 95.23% <ø> (ø) ⬆️
src/dirs.js 97.18% <ø> (ø) ⬆️
src/to.js 100% <ø> (ø) ⬆️
src/uniq.js 100% <ø> (ø) ⬆️
src/head.js 100% <ø> (ø) ⬆️
src/echo.js 100% <ø> (ø) ⬆️
src/pwd.js 100% <ø> (ø) ⬆️
... and 19 more

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 9077f41...d78b1cf. Read the comment docs.

Copy link
Member

@nfischer nfischer left a comment

Choose a reason for hiding this comment

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

Thanks for cleaning this up, this looks a lot better!

@freitagbr feel free to chime in, otherwise I'll merge.

@nfischer nfischer merged commit 9035b27 into shelljs:master Feb 20, 2018
@nfischer
Copy link
Member

Thanks, I edited commit message.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants