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

doc: add note for platform specific flags `fs.open()` #6136

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
7 participants
@eljefedelrodeodeljefe
Copy link
Contributor

commented Apr 9, 2016

Checklist
  • tests and code linting passes
  • a test and/or benchmark is included
  • documentation is changed or added
  • the commit message follows commit guidelines
Affected core subsystem(s)

doc

Description of change

add note for platform specific flags fs.open()

closes #3643
E.g. fs.open('', 'a+', console.log)

@mscdex mscdex added doc fs labels Apr 9, 2016

@mscdex

View changes

doc/api/fs.markdown Outdated
@@ -783,6 +783,22 @@ On Linux, positional writes don't work when the file is opened in append mode.
The kernel ignores the position argument and always appends the data to
the end of the file.

_Note: The behavior of `fs.open()` is platform specific for some flags. As such,
opening a directory on OS X with `'a+''`, like in the example below, will return

This comment has been minimized.

Copy link
@mscdex

mscdex Apr 9, 2016

Contributor

There's an extra ' in the flag on this line.

This comment has been minimized.

Copy link
@eljefedelrodeodeljefe

eljefedelrodeodeljefe Apr 9, 2016

Author Contributor

Thx. Fixed

@silverwind

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2016

Linux gives EISDIR too, might consider adding this information. Otherwise LGTM.

@eljefedelrodeodeljefe

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2016

Amended Linux. Change wording bit around there. Thx.

@silverwind

View changes

doc/api/fs.markdown Outdated
returned._

```js
// OS X

This comment has been minimized.

Copy link
@silverwind

silverwind Apr 9, 2016

Contributor

Might wanna add OS X and Linux

@silverwind

View changes

doc/api/fs.markdown Outdated
// => [Error: EISDIR: illegal operation on a directory, open <directory>]
})

// Windows

This comment has been minimized.

Copy link
@silverwind

silverwind Apr 9, 2016

Contributor

Windows and FreeBSD

@silverwind

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2016

LGTM with nits.

@eljefedelrodeodeljefe

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2016

Done.

@benjamingr

This comment has been minimized.

Copy link
Member

commented Apr 10, 2016

LGTM

By the way - I like the work you've been doing with covering all the caveats @eljefedelrodeodeljefe but I'm worried it might confuse new users by blasting them with a lot of information not all of them care about.

I wonder if we should collapse notes by default or only show the first line or something like that. I'm CCing @nodejs/documentation for discussion about that but feel free to move the discussion to the docs repo if it starts derailing things here.

@eljefedelrodeodeljefe

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2016

That's a very good idea. I think Chris is calling for a meeting soon. Let's put this on the agenda then. We had similar things I would call "added dimension" like the es6/es5 switch. Thx

@jasnell

This comment has been minimized.

Copy link
Member

commented Apr 10, 2016

LGTM

@stevemao

This comment has been minimized.

Copy link
Member

commented Apr 19, 2016

I'm worried it might confuse new users by blasting them with a lot of information not all of them care about.

I'd rather see more information than very terse descriptions. But I think it should be terse (one sentence) with links if the problem is not node.js itself.

Maybe we could put those extra information in a collapsible section.

@eljefedelrodeodeljefe

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2016

I like the direction it takes, @stevemao. @benjamingr care for landing?

@jasnell

This comment has been minimized.

Copy link
Member

commented Apr 19, 2016

It may be a worthwhile exercise to have a doc/topics page that discusses the platform specific variances that exist in the fs module as there are (unfortunately) quite a few. Things such as the differences in fs.watch support, filename encoding differences, flags, etc. Separating that discussion out to a separate doc would help keep us from having to load the API doc down too much and would give us a place to link off too for more discussion. /cc @nodejs/documentation.

@jasnell

This comment has been minimized.

Copy link
Member

commented Apr 21, 2016

This will need a rebase since the commit renaming the files landed.

doc: add note for platform specific flags `fs.open()`
closes #3643

E.g. fs.open('<directory>', 'a+', console.log)
@eljefedelrodeodeljefe

This comment has been minimized.

Copy link
Contributor Author

commented Apr 21, 2016

rebased

@jasnell

This comment has been minimized.

Copy link
Member

commented Apr 21, 2016

Awesome, thank you. One last round of review from @nodejs/documentation?

@silverwind

This comment has been minimized.

Copy link
Contributor

commented Apr 21, 2016

Still LGTM

jasnell added a commit that referenced this pull request Apr 22, 2016

doc: add note for platform specific flags `fs.open()`
Note describing platform specific differences in fs.open

E.g. fs.open('<directory>', 'a+', console.log)

Fixes: #3643
PR-URL: #6136
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell

This comment has been minimized.

Copy link
Member

commented Apr 22, 2016

Landed in ae991e7

@jasnell jasnell closed this Apr 22, 2016

joelostrowski added a commit to joelostrowski/node that referenced this pull request Apr 25, 2016

doc: add note for platform specific flags `fs.open()`
Note describing platform specific differences in fs.open

E.g. fs.open('<directory>', 'a+', console.log)

Fixes: nodejs#3643
PR-URL: nodejs#6136
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

jasnell added a commit that referenced this pull request Apr 26, 2016

doc: add note for platform specific flags `fs.open()`
Note describing platform specific differences in fs.open

E.g. fs.open('<directory>', 'a+', console.log)

Fixes: #3643
PR-URL: #6136
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins

This comment has been minimized.

Copy link
Member

commented Jun 1, 2016

@eljefedelrodeodeljefe does this apply to v4.x?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.