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 annotation to writeFile data as Object #39167

Conversation

JakobJingleheimer
Copy link
Contributor

@JakobJingleheimer JakobJingleheimer commented Jun 27, 2021

Fixes: #39152

@github-actions github-actions bot added doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system. labels Jun 27, 2021
doc/api/fs.md Outdated Show resolved Hide resolved
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

I'm not sure using * is a great idea, I don't think we're using this anywhere else, and I personally find it confusing. You could simply put the info in the body of the docs, like this:

A `TypeError` will be thrown if `size` is not a number.

doc/api/fs.md Outdated Show resolved Hide resolved
doc/api/fs.md Outdated Show resolved Hide resolved
doc/api/fs.md Outdated Show resolved Hide resolved
doc/api/fs.md Outdated Show resolved Hide resolved
@JakobJingleheimer
Copy link
Contributor Author

Consolidating the various * threads to the main discussion so they don't get lost:

Sorry, I didn't mean a literal asterisk.

Oh 😅

I would just add the language used in #34993

@mscdex @aduh95 I included the footnote marker instead of duplicating the wording everywhere so it could be consolidated (since it probably has a single implementation source under the hood—meaning they would all change together).

Putting myself in the user's shoes, I figure if I saw … | <Stream> | <Object>*, I would put together that there is some note somewhere specifically related to data as an Object. Since writeFileSync()'s description basically just directs to writeFile() but had no note, I would follow the redirect to writeFile and expect to find it there (and there it indeed is).

Regarding the * itself, I think it's important to have some kind of visual cue on/near the type defs so a user will see it and know there's something to look for; just chucking a gotcha in the body seems insufficient to me. Without, I think it would be reasonable for a user who is only looking to see what types can be used for an argument to look only at the type def, see nothing, and stop here.

In terms of which marker to use, my preference would be superscript numbers because it's robust (supporting an unlimited number of them, whereas symbols like *†‡ quickly run out). So something like:

foo {string} ¹ Tellus mauris a diam maecenas.
bar {Buffer|Stream|Object} ² Velit aliquet sagittis id consectetur.

Ultrices in iaculis nunc sed augue. Ipsum suspendisse ultrices gravida dictum
fusce ut placerat orci nulla. Nec nam aliquam sem et tortor consequat id porta.
Nibh tellus molestie nunc non blandit massa. Pharetra massa massa ultricies mi.
Netus et malesuada fames ac turpis egestas integer.

1. The first footnote here
2. The second footnote here

@mscdex
Copy link
Contributor

mscdex commented Jun 27, 2021

instead of duplicating the wording everywhere

IMO I don't think that's a problem. There is a lot of duplicated wording in various sections of the node docs.

In general I think it's no different than clarifying anything else about the signature or behavior of various APIs. The end user will need to be reading the function description anyway. If we're going to be changing how we convey certain types of information in the node API documentation, we should probably do so on a wider scale with a solid plan. For now though I think it's enough to just add the missing wording where necessary, even if it's duplicated.

@JakobJingleheimer
Copy link
Contributor Author

The end user will need to be reading the function description anyway.

That is not true for the example I gave above.

But yes, I agree: if there is no precedent for footnotes, best not to start randomly now. I'll update to duplicating the wording and leave the DRYing for later.

@JakobJingleheimer JakobJingleheimer force-pushed the fs-doc-types-annotation branch 3 times, most recently from 010ea71 to 941010d Compare June 29, 2021 15:54
doc/api/fs.md Outdated
@@ -5094,6 +5105,9 @@ changes:
* `encoding` {string}
* Returns: {number} The number of bytes written.

If `string` is a plain object, like all variations of `writeFile`, it must have
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'm not sure if this is applicable, but Object is listed in the types, so I guessed yes. Wanted to specifically call out just in case.

@JakobJingleheimer
Copy link
Contributor Author

nudge @mscdex @aduh95

doc/api/fs.md Outdated Show resolved Hide resolved
@JakobJingleheimer
Copy link
Contributor Author

@aduh95 🙏 I don't have access to merge PRs yet (not until after my first PR lands, if I recall correctly). Could you assist?

@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels Jul 7, 2021
@github-actions github-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 7, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Jul 7, 2021

Landed in 9257372...6a4b4ce

@github-actions github-actions bot closed this Jul 7, 2021
nodejs-github-bot pushed a commit that referenced this pull request Jul 7, 2021
Fixes: #39152

PR-URL: #39167
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this pull request Jul 11, 2021
Fixes: #39152

PR-URL: #39167
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this pull request Sep 4, 2021
Fixes: #39152

PR-URL: #39167
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

writeFileSync data as Object throws type error despite docs listing Object as a supported type
3 participants