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, tools: formalize, unify and codify default values #19737

Closed
wants to merge 2 commits into from
Closed

doc, tools: formalize, unify and codify default values #19737

wants to merge 2 commits into from

Conversation

vsemozhetbyt
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt commented Apr 2, 2018

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

Status Quo

  1. The format of default values is defined in STYLE_GUIDE.md. But the current state of sources is not unified: many default values are stated in informal ways or with tiny deviations.

  2. tools/doc/json.js has an obsolete RegExp to detect the default values in parameter lists. Because of this, only one default field is added to JSON docs, and even this is detected wrongly: this source produces this item:

{
  "textRaw": "`undefined` (default): use [`http.globalAgent`][] for this host and port. ",
  "name": "undefined",
- "default": "",
- "desc": ": use [`http.globalAgent`][] for this host and port."
}

insstead of:

{
  "textRaw": "`undefined` (default): use [`http.globalAgent`][] for this host and port. ",
  "name": "undefined",
+ "desc": "(default): use [`http.globalAgent`][] for this host and port."
}

Fixing strategy

  1. The first commit formalizes and unifies doc sources: informal definitions are replaced by formal ones, deviating nits in the formal definitions are unified (definitions are moved to the end, backticks and periods are added, duplicate info is removed etc.)

  2. The second commit codifies default values format: periods are added to STYLE_GUIDE.md and RegExp in tools/doc/json.js is replaced by the actual one.

Results

  • Doc sources are unified and often fixed.
  • ~340 default values are moved from description blobs to default value fields in JSON docs.
  • The mentioned false-positive case is fixed.

JSON example of old and new state in diff:

{
  "textRaw": "`code` {integer} The exit code. **Default:** `0`. ",
  "name": "code",
  "type": "integer",
- "desc": "The exit code. **Default:** `0`.",
  "optional": true
}

{
  "textRaw": "`code` {integer} The exit code. **Default:** `0`. ",
  "name": "code",
+ "default": "`0`",
  "type": "integer",
+ "desc": "The exit code.",
  "optional": true
}

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Apr 2, 2018
@vsemozhetbyt vsemozhetbyt added the tools Issues and PRs related to the tools directory. label Apr 2, 2018
@vsemozhetbyt
Copy link
Contributor Author

@vsemozhetbyt vsemozhetbyt added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 2, 2018
@vsemozhetbyt
Copy link
Contributor Author

cc @nodejs/documentation

@@ -16,8 +16,8 @@
* Avoid personal pronouns in reference documentation ("I", "you", "we").
* Personal pronouns are acceptable in colloquial documentation such as guides.
* Use gender-neutral pronouns and gender-neutral plural nouns.
* OK: "they", "their", "them", "folks", "people", "developers"
* NOT OK: "his", "hers", "him", "her", "guys", "dudes"
* OK: "they", "their", "them", "folks", "people", "developers".
Copy link
Member

Choose a reason for hiding this comment

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

This is not a sentence so I don't think a . at the end makes sense here. Same for the line below.

@@ -43,7 +43,7 @@
* For illustrations, prefer SVG to other assets. When SVG is not feasible,
please keep a close eye on the filesize of the asset you're introducing.
* For code blocks:
* Use language aware fences. ("```js")
* Use language aware fences. ("```js").
Copy link
Member

Choose a reason for hiding this comment

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

Remove the period after fences if we're putting one after the parenthetical.

* Function returns should use the following format:
* <code>* Returns: {type|type2} Optional description.</code>
* E.g. <code>* Returns: {AsyncHook} A reference to `asyncHook`.</code>
* Use official styling for capitalization in products and projects.
* OK: JavaScript, Google's V8
* NOT OK: Javascript, Google's v8
* OK: JavaScript, Google's V8.
Copy link
Member

Choose a reason for hiding this comment

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

Again, this is a list and not a sentence, so I don't think it warrants punctuation at the end. Same for the line below.

@@ -469,7 +469,7 @@ parameter is an instance of an [`Error`][] then it will be thrown instead of the
<!-- YAML
added: v0.1.21
-->
* `message` {any} **Default:** `'Failed'`
* `message` {any} **Default:** `'Failed'`.
Copy link
Member

@Trott Trott Apr 2, 2018

Choose a reason for hiding this comment

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

I'll stop after this one, but basically I don't think that we should put periods after values that aren't otherwise part of a sentence or even a phrase.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will undo,

Copy link
Contributor Author

@vsemozhetbyt vsemozhetbyt Apr 2, 2018

Choose a reason for hiding this comment

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

I've removed the periods if:

  • default value statement is not a sentence;
  • default value statement is not after a sentence (is not after a period).

@vsemozhetbyt vsemozhetbyt removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 2, 2018
@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Apr 2, 2018

@Trott
Copy link
Member

Trott commented Apr 2, 2018

Still not a fan of all the added periods, but at least they're consistent, so non-blocking objection. But if you want my opinion on where periods should and shouldn't go, let me know. :-D

@vsemozhetbyt
Copy link
Contributor Author

As a not native speaker, I definitely need to understand more) So if you add some notes, I will edit accordingly.

@Trott
Copy link
Member

Trott commented Apr 2, 2018

As a not native speaker, I definitely need to understand more) So if you add some notes, I will edit accordingly.

I'm having a lot of trouble finding examples in documentation (of other languages) or in style guides to support the approach I would suggest. And what I am finding basically amounts to "There are no definite rules other than to be consistent." So maybe best to just go with the approach you have here.

(I would expect that **Default**: 'some value' would never be followed by a period. Rather than adding a period everywhere for consistency, I'd remove the period everywhere for consistency. But like I said, I can't find a lot of support for that anywhere, and I'm not sure "because @Trott likes it" is a good enough reason!)

@Trott
Copy link
Member

Trott commented Apr 2, 2018

Thinking more on it: "Always add a period" does have the advantage that it requires no debate/decision to be made. It's simple. I think I may be coming around to your approach. :-D

@vsemozhetbyt
Copy link
Contributor Author

Human language is a live tricky thing)
So we can leave it as is for now unless somebody would have a strong opinion otherwise?)

@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Apr 2, 2018

What I was thinking about the second edition:

* `param` {type} **Default**: `some value`

is hardly a sentence or phrase, more likely a tag, a formal construction.
While

* `param` {type} This a foo that makes bar be baz. **Default**: `some value`.

is a story, a narrative, a natural flow, and its momentum makes even partial phrases be more sentence-like, as a sentence with an ellipsis.

vsemozhetbyt added a commit that referenced this pull request Apr 4, 2018
PR-URL: #19737
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
@vsemozhetbyt
Copy link
Contributor Author

Landed in 237cbe1

@vsemozhetbyt vsemozhetbyt deleted the doc-tools-json-default branch April 4, 2018 09:34
@targos
Copy link
Member

targos commented Apr 4, 2018

Should this be backported to v9.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

@vsemozhetbyt
Copy link
Contributor Author

Backport to v9: #19793

targos pushed a commit that referenced this pull request Apr 6, 2018
Backport-PR-URL: #19793
PR-URL: #19737
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
@MylesBorins
Copy link
Contributor

Should this be backported to v8.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

@vsemozhetbyt
Copy link
Contributor Author

Backport to v8: #22388

MylesBorins pushed a commit that referenced this pull request Sep 6, 2018
Backport-PR-URL: #22388
PR-URL: #19737
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Sep 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants