FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

doc: fix JSON generation for aliased methods by TimothyGu · Pull Request #4871 · nodejs/node · GitHub

/ node Public

doc: fix JSON generation for aliased methods - #4871

Closed
TimothyGu wants to merge 1 commit into
nodejs:masterfrom
TimothyGu:json-doc
ConversationCommits (1)ChecksFiles changed
Closed

doc: fix JSON generation for aliased methods#4871
TimothyGu wants to merge 1 commit into
nodejs:masterfrom
TimothyGu:json-doc

Conversation

TimothyGu commented Jan 26, 2016

Copy link
Copy Markdown
Member

For example assert/assert.ok currently has the following signature:

    "signatures": [
      {
        "params": [
          {
            "name": "value"
          },
          {
            "name": "message])"
          },
          {
            "name": "assert.ok(value"
          },
          {
            "name": "message",
            "optional": true
          }
        ]
      }
    ]

While the heading reads

assert(value[, message]), assert.ok(value[, message])

Split them into two sections to make it working.

mscdex added doc Issues and PRs related to the documentations. assert Issues and PRs related to the assert subsystem. labels Jan 26, 2016

evanlucas commented Jan 26, 2016

Copy link
Copy Markdown
Contributor

/cc @nodejs/documentation

eljefedelrodeodeljefe commented Jan 26, 2016

Copy link
Copy Markdown
Contributor

Wouldn't it be better to fix it during JSON generation? Because the markdown renders fine and a change there doesn't seem to be something we should wish for, imo. What's your use case?

tflanagan commented Jan 26, 2016

Copy link
Copy Markdown
Contributor

There is precedence for this type of change:

https://nodejs.org/api/https.html#https_server_listen_handle_callback

Just a friendly reminder that the whole doc generating process is pretty fragile, so I would +1 this change, as it already exists in the HTTPS docs.

eljefedelrodeodeljefe commented Jan 26, 2016

Copy link
Copy Markdown
Contributor

Okay. Could we have a See assert.ok() for details. then? Because the difference is the that srv.listen() is one heading level deeper.

TimothyGu commented Jan 26, 2016

Copy link
Copy Markdown
Member Author

Hi,

https://nodejs.org/api/buffer.html#buffer_buf_readdoublebe_offset_noassert

is not deeper and it uses the same style.

TimothyGu commented Jan 26, 2016

Copy link
Copy Markdown
Member Author

Or actually it is. But I still don't see how that affects the argument.

eljefedelrodeodeljefe commented Jan 26, 2016

Copy link
Copy Markdown
Contributor

It's bout consistency and readability. Also prettiness and semantics, since a heading without content looks like a bug to me. Even if there were precedence.

tflanagan commented Jan 26, 2016

Copy link
Copy Markdown
Contributor

@eljefedelrodeodeljefe, you will find this style all over the docs. It is not a bug. It is by design.

The only argument against this is resorting the documentation which would cause duplicate documentation because we will have to move assert.ok down in the list.

As I have done this before, I am sure it is fine now.

eljefedelrodeodeljefe commented Jan 26, 2016

Copy link
Copy Markdown
Contributor

Then make the case. -0

tflanagan commented Jan 26, 2016

Copy link
Copy Markdown
Contributor

@eljefedelrodeodeljefe I would argue the case is FOR consistency and readability.

tbh, this was a mistake on my part in missing this when I reordered all the docs alphabetically, this should've already happened.

TimothyGu commented Jan 26, 2016

Copy link
Copy Markdown
Member Author

@tflanagan that's a good point. How about I move ok to where it should be and link assert() to assert.ok()? This way it reduces redundancy and keeps the alphabetical order.

On the doc toolchain being fragile, I totally agree.

@eljefedelrodeodeljefe there are code in the toolchain to specifically handle this sort of headings: https://github.com/nodejs/node/blob/master/tools/doc/json.js#L38-L39. So I'd argue that the headings are like this by design.

doc: fix JSON generation for aliased methods
9a8f085 
Currently assert/assert.ok currently has the following signature:

    "signatures": [
      {
        "params": [
          {
            "name": "value"
          },
          {
            "name": "message])"
          },
          {
            "name": "assert.ok(value"
          },
          {
            "name": "message",
            "optional": true
          }
        ]
      }
    ]

The heading reads

    assert(value[, message]), assert.ok(value[, message])

Split them into two sections to make it working.

TimothyGu commented Jan 30, 2016

Copy link
Copy Markdown
Member Author

I have just splitted the section into two, so that the alphabetical order is preserved.

chrisdickinson commented Jan 30, 2016

Copy link
Copy Markdown
Contributor

This LGTM. Making the headings more regular is great (even if it is to fix the JSON generator, which is pretty neglected!)

thefourtheye reviewed Jan 30, 2016
View reviewed changes
Comment thread doc/api/assert.markdown

thefourtheye Jan 30, 2016

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

All the other references in this document use the format assert.ok.

TimothyGu Jan 30, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This isn't true. See e.g. https://github.com/nodejs/node/blob/master/doc/api/assert.markdown#assertdoesnotthrowblock-error-message

thefourtheye Jan 30, 2016

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

All I am saying is that, it should be consistent.

eljefedelrodeodeljefe commented Jan 30, 2016

Copy link
Copy Markdown
Contributor

LGTM now.

Trott pushed a commit to Trott/io.js that referenced this pull request Jan 30, 2016
doc: fix JSON generation for aliased methods
27c8b73 
Currently assert/assert.ok currently has the following signature:

    "signatures": [
      {
        "params": [
          {
            "name": "value"
          },
          {
            "name": "message])"
          },
          {
            "name": "assert.ok(value"
          },
          {
            "name": "message",
            "optional": true
          }
        ]
      }
    ]

The heading reads

    assert(value[, message]), assert.ok(value[, message])

Split them into two sections to make it working.

PR-URL: nodejs#4871
Reviewed-By: Chris Dickinson <christopher.s.dickinson@gmail.com>

Trott commented Jan 30, 2016

Copy link
Copy Markdown
Member

Landed in 27c8b73

Trott closed this Jan 30, 2016
TimothyGu deleted the json-doc branch January 30, 2016 19:17

jasnell commented Feb 1, 2016

Copy link
Copy Markdown
Member

lts-watch label applied

rvagg pushed a commit that referenced this pull request Feb 8, 2016
doc: fix JSON generation for aliased methods
17888b1 
Currently assert/assert.ok currently has the following signature:

    "signatures": [
      {
        "params": [
          {
            "name": "value"
          },
          {
            "name": "message])"
          },
          {
            "name": "assert.ok(value"
          },
          {
            "name": "message",
            "optional": true
          }
        ]
      }
    ]

The heading reads

    assert(value[, message]), assert.ok(value[, message])

Split them into two sections to make it working.

PR-URL: #4871
Reviewed-By: Chris Dickinson <christopher.s.dickinson@gmail.com>
rvagg mentioned this pull request Feb 8, 2016
Closed

MylesBorins commented Feb 17, 2016

Copy link
Copy Markdown
Contributor

This PR will have to wait for a backlog of doc fixes to land before we can land it in LTS

Knighton910 commented Feb 17, 2016

Copy link
Copy Markdown

👍

MylesBorins pushed a commit that referenced this pull request Feb 22, 2016
doc: fix JSON generation for aliased methods
7967f51 
Currently assert/assert.ok currently has the following signature:

    "signatures": [
      {
        "params": [
          {
            "name": "value"
          },
          {
            "name": "message])"
          },
          {
            "name": "assert.ok(value"
          },
          {
            "name": "message",
            "optional": true
          }
        ]
      }
    ]

The heading reads

    assert(value[, message]), assert.ok(value[, message])

Split them into two sections to make it working.

PR-URL: #4871
Reviewed-By: Chris Dickinson <christopher.s.dickinson@gmail.com>
This was referenced Jul 7, 2023
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Merged
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
snyk-io Bot mentioned this pull request Oct 21, 2024
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

assert Issues and PRs related to the assert subsystem. doc Issues and PRs related to the documentations.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Back | FazBrowse Home | New Git URL