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.

Sign up for GitHub

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

Jump to bottom

doc: fs.mkdir('/') throws EPERM on Windows #25340

Closed
wants to merge 1 commit into from
Closed

doc: fs.mkdir('/') throws EPERM on Windows #25340

wants to merge 1 commit into from

Conversation

coreyfarrell
Copy link
Member

Fixes: #25110

Checklist

CC @bcoe @nodejs/fs

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system. labels Jan 4, 2019
Trott
Trott requested changes Jan 4, 2019
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Hi, @coreyfarrell! Welcome and thanks for the pull request.

I've left a comment with a requested change. It wraps the text at 80 characters, which you should have received an error about if you ran make lint-md (fast) or make test (slow). It also changes the wording, adds punctuation, and styling. Please take a look.

doc/api/fs.md Outdated Show resolved Hide resolved
@Trott Trott added the windows Issues and PRs related to the Windows platform. label Jan 4, 2019
@coreyfarrell coreyfarrell mentioned this pull request Jan 4, 2019
Merged
@coreyfarrell
Copy link
Member Author

@MylesBorins This documentation update is intended for v10.x and v11.x in addition to master.

I assume you don't want me to open PR's for each branch but please let me know if I need to take additional actions or how to handle situations like this in the future.

Trott
Trott reviewed Jan 4, 2019
View reviewed changes
doc/api/fs.md Outdated Show resolved Hide resolved
Trott
Trott approved these changes Jan 4, 2019
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

LGTM with or without the nit-pick suggestion I just left.

@Trott
Copy link
Member

Trott commented Jan 4, 2019

I assume you don't want me to open PR's for each branch

Someone will cherry-pick the commit to the other branches. If the cherry-pick does not work because of a conflict, they may leave a comment here asking you to open a backport PR. There's nothing to do at this time, though.

bcoe
bcoe approved these changes Jan 4, 2019
View reviewed changes
Copy link
Contributor

@bcoe bcoe 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 adding this note @coreyfarrell take or leave my comma preferences.

doc/api/fs.md
@@ -2212,6 +2212,15 @@ fs.mkdir('/tmp/a/apple', { recursive: true }, (err) => {
});
```

On Windows, using `fs.mkdir()` on the root directory even with recursion will
Copy link
Contributor

@bcoe bcoe Jan 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

the nit picky editor in me wonders whether this should be:

Using `fs.mkdir()` on the root directory, even with recursion, will
result in an error on Windows:

Copy link
Member Author

Choose a reason for hiding this comment

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

My preference is for the fact that it's platform specific to be first but I'm willing to do whatever the consensus is.

Copy link
Member

Choose a reason for hiding this comment

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

This maybe?

On Windows, using `fs.mkdir()` on the root directory, even with recursion, will
result in an error:

@bcoe bcoe added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 6, 2019
@bcoe
Copy link
Contributor

bcoe commented Jan 6, 2019

ci: https://ci.nodejs.org/job/node-test-commit/24828/

bcoe pushed a commit that referenced this pull request Jan 6, 2019
@coreyfarrell @bcoe
doc: fs.mkdir('/') throws EPERM on Windows
e7c6f4f 
Fixes: #25110

PR-URL: #25340
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ben Coe <bencoe@gmail.com>
@bcoe
Copy link
Contributor

bcoe commented Jan 6, 2019 •
edited
Loading

Landed in e7c6f4f

@bcoe bcoe closed this Jan 6, 2019
@bcoe
Copy link
Contributor

bcoe commented Jan 6, 2019

@coreyfarrell congrats on your first contribution to Node.js 🎉

@coreyfarrell coreyfarrell deleted the fs-mkdir-docs branch January 6, 2019 22:06
addaleax pushed a commit that referenced this pull request Jan 9, 2019
@coreyfarrell @addaleax
doc: fs.mkdir('/') throws EPERM on Windows
f8dcbba 
Fixes: #25110

PR-URL: #25340
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ben Coe <bencoe@gmail.com>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
@coreyfarrell @refack
doc: fs.mkdir('/') throws EPERM on Windows
2aa197b 
Fixes: nodejs#25110

PR-URL: nodejs#25340
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ben Coe <bencoe@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Jan 16, 2019
Merged
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
@coreyfarrell @BridgeAR
doc: fs.mkdir('/') throws EPERM on Windows
b1023ba 
Fixes: nodejs#25110

PR-URL: nodejs#25340
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ben Coe <bencoe@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2019
Merged
BethGriggs pushed a commit that referenced this pull request Apr 28, 2019
@coreyfarrell @BethGriggs
doc: fs.mkdir('/') throws EPERM on Windows
4717767 
Fixes: #25110

PR-URL: #25340
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ben Coe <bencoe@gmail.com>
@BethGriggs BethGriggs mentioned this pull request May 1, 2019
Merged
BethGriggs pushed a commit that referenced this pull request May 10, 2019
@coreyfarrell @BethGriggs
doc: fs.mkdir('/') throws EPERM on Windows
757cf4d 
Fixes: #25110

PR-URL: #25340
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ben Coe <bencoe@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 16, 2019
@coreyfarrell @MylesBorins
doc: fs.mkdir('/') throws EPERM on Windows
ef6e4f1 
Fixes: #25110

PR-URL: #25340
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ben Coe <bencoe@gmail.com>
This was referenced May 29, 2019
Closed
Closed
Closed
Closed
Closed
This was referenced Aug 9, 2020
Open
Open
Open
Open
Open
Open
Merged
@snyk-bot snyk-bot mentioned this pull request Oct 29, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Trott Trott Trott approved these changes

@lpinca lpinca lpinca approved these changes

@bcoe bcoe bcoe approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system. windows Issues and PRs related to the Windows platform.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@coreyfarrell @Trott @bcoe @lpinca @nodejs-github-bot