Skip to content

error: document removed error codes #22100

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

Closed
SirR4T wants to merge 9 commits into from
Closed

error: document removed error codes #22100

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
0546c6a
[WIP] error: documenting removed error codes.
Aug 2, 2018
25a533f
doc: fix linter issues for legacy error codes
Aug 6, 2018
208cb4f
doc: Fix linter issues in errors.md
Aug 6, 2018
9b53c58
doc: add YAML parsers for removed sections
Aug 7, 2018
dd31ede
doc: update list of deprecated / removed error codes
SirR4T Aug 23, 2018
1ad00f2
docs: fix typos in error docs
SirR4T Aug 24, 2018
e77d298
doc: errors that have been removed but not released
SirR4T Aug 24, 2018
e536153
error: reorder errors which were never released
SirR4T Aug 25, 2018
f10d321
doc: removed hard coded handles where unnecessary
SirR4T Aug 26, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
doc: add YAML parsers for removed sections
  • Loading branch information
Sarat Addepalli @SirR4T
Sarat Addepalli authored and SirR4T committed Aug 23, 2018
commit 9b53c58b05182093e8d9d5b771b9c73485bad79f
2 changes: 2 additions & 0 deletions doc/api/errors.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1855,6 +1855,7 @@ A module file could not be resolved while attempting a [`require()`][] or
### ERR_FS_WATCHER_ALREADY_STARTED
<!-- YAML
added: v10.0.0

@BridgeAR BridgeAR Aug 6, 2018

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.

It would be nice if we add a "removed: v10.x.x". I guess that might not be parsed properly tough. @vsemozhetbyt do you have a suggestion for this?

@vsemozhetbyt vsemozhetbyt Aug 6, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We may add the case for removing to adding and deprecating. It seems these two function need to be updated accordingly:

node/tools/doc/common.js

Line 14 in 2bea9ce

function extractAndParseYAML(text) {

node/tools/doc/html.js

Line 258 in 2bea9ce

function parseYAML(text) {

(Just grepping 'added' or 'deprecated' and adding a similar case may do)

@SirR4T SirR4T Aug 7, 2018 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@BridgeAR added these sections (thanks @vsemozhetbyt, for the pointers), now shows up as
screen shot 2018-08-07 at 10 34 04 am

Although cc: @joyeecheung , because it seems to me she has some reservations against saying "removed in v10.x.x" .

@SirR4T SirR4T Aug 8, 2018

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Also, should there be a short explanation on why this error code was removed? Something on the lines of

Like fs.watchFile(), this situation is now silently ignored.

or some such.

@joyeecheung joyeecheung Aug 8, 2018 •
edited
Loading

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.

@SirR4T I think it's OK to have removed in, but the version number should be accurate. In the case of ERR_FS_WTCHER_ALREADY_STARTED, it has never been released in any version, since the commit that added it (6c25f2e) and the commit that removed it (301f6cc) are both present from v10.0.0...v10.8.0 - which means when v10.0.0 was out, the error was not even there. Therefore, this error code has only appeared in nightly and canary releases.

@vsemozhetbyt vsemozhetbyt Aug 8, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If we do add a short explanation, we can make this in changes blocks.

@SirR4T SirR4T Aug 22, 2018

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

got it. If the error codes never actually touched the releases, what should the description look like? is having both added: v10.0.0 and removed: v10.0.0 ok?

@SirR4T SirR4T Aug 22, 2018 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@joyeecheung : also, I have added versions added and versions removed here, for all the commits which either added or removed the error codes. Eyeballing the versions, it seems to me that the wherever github mentions vA.a.a .. vB.b.b, we should be mentioning the vB.b.b in the docs. (For both added, as well as removed.) will that be correct?

@joyeecheung joyeecheung Aug 23, 2018 •
edited
Loading

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.

I think annotating them as added: v10.0.0 removed: v10.0.0 is more confusing than helpful. A note in the docs about that would be enough, maybe something like:

This error code has never been released and only existed in nightly builds.

removed: v10.8.0
-->
An attempt was made to start a watcher returned by `fs.watch()` that has
already been started.
Expand All @@ -1863,6 +1864,7 @@ already been started.
### ERR_FS_WATCHER_NOT_STARTED
<!-- YAML
added: v10.0.0
removed: v10.8.0
-->

An attempt was made to initiate operations on a watcher returned by
Expand Down
4 changes: 4 additions & 0 deletions tools/doc/common.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,10 @@ function extractAndParseYAML(text) {
meta.deprecated = arrify(meta.deprecated);
}

if (meta.removed) {
meta.removed = arrify(meta.removed);

@joyeecheung joyeecheung Aug 26, 2018

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.

Is this needed for this PR? From what I can tell we only used single removed fields here?

@SirR4T SirR4T Aug 26, 2018

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I was blindly copy pasting wherever added occurred.

Maybe not for this particular PR, but if we're planning to add removed as a feature for api docs, would rather it supports semver-minors as well, like added and deprecated.

@joyeecheung joyeecheung Aug 26, 2018

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.

@SirR4T Ah, sorry, I thought we already had removed before, looks like it is new in this PR...

}

meta.changes = meta.changes || [];

return meta;
Expand Down
10 changes: 9 additions & 1 deletion tools/doc/html.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,7 @@ function parseYAML(text) {

const added = { description: '' };
const deprecated = { description: '' };
const removed = { description: '' };

if (meta.added) {
added.version = meta.added.join(', ');
Expand All @@ -276,9 +277,15 @@ function parseYAML(text) {
`<span>Deprecated since: ${deprecated.version}</span>`;
}

if (meta.removed) {
removed.version = meta.removed.join(', ');
removed.description = `<span>Removed in: ${removed.version}</span>`;
}

if (meta.changes.length > 0) {
if (added.description) meta.changes.push(added);
if (deprecated.description) meta.changes.push(deprecated);
if (removed.description) meta.changes.push(removed);

meta.changes.sort((a, b) => versionSort(a.version, b.version));

Expand All @@ -299,7 +306,8 @@ function parseYAML(text) {

result += '</table>\n</details>\n';
} else {
result += `${added.description}${deprecated.description}\n`;
result += `${added.description}${deprecated.description}` +
`${removed.description}\n`;
}

if (meta.napiVersion) {
Expand Down