Skip to content

Add support for removing fifos#687

Merged
freitagbr merged 4 commits intomasterfrom
rm-fifos
Mar 8, 2017
Merged

Add support for removing fifos#687
freitagbr merged 4 commits intomasterfrom
rm-fifos

Conversation

@freitagbr
Copy link
Copy Markdown
Contributor

Adds support for removing fifos.

Fixes #617

Copy link
Copy Markdown
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.

LGTM aside from my one comment


function mkfifo(dir) {
if (process.platform !== 'win32') {
const fifo = `${dir}/fifo`;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you use regular string concatenation? This doesn't work for v0.12

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@nfischer
Copy link
Copy Markdown
Member

nfischer commented Mar 8, 2017

FFTM if CI passes

@codecov-io
Copy link
Copy Markdown

Codecov Report

Merging #687 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #687      +/-   ##
==========================================
+ Coverage   94.32%   94.34%   +0.01%     
==========================================
  Files          33       33              
  Lines        1180     1184       +4     
==========================================
+ Hits         1113     1117       +4     
  Misses         67       67
Impacted Files Coverage Δ
src/rm.js 100% <100%> (ø)
src/cp.js 90.43% <0%> (+0.34%)

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 b6b2cd8...2c4b3b0. Read the comment docs.

@freitagbr
Copy link
Copy Markdown
Contributor Author

CI failures are unrelated.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants