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

tools: parse documentation metadata (take 2) #6495

Closed
wants to merge 6 commits into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented May 1, 2016

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

tools, doc

Description of change

This is a continuation of #3867. Original description:


This commit introduces the Documentation YAML metadata concept to the
documentation.

  • Parses added or Added for HTML
  • Parses all data for JSON

This is in prelude to #3713.

Markup would look like this:

## assert(value[, message]), assert.ok(value[, message])
<!-- YAML
added: v0.10.0
-->

Changed/additionally included in this PR:

  • Rebased the original commit to master, and applied fixes so linting passes with the current rules.
  • Allow the added: attribute to be a collection of versions rather than a single one, since semver-minors can trickle down to previous major versions.
  • Add a deprecated: attribute.
  • As an example, add these attributes in buffer.md to show how this will work in real life.

/cc @nodejs/documentation

@addaleax addaleax added doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory. labels May 1, 2016
@Fishrock123
Copy link
Contributor

I'm really not sure we should be checking this into core; could it be part of the website tooling instead?

@jasnell
Copy link
Member

jasnell commented May 1, 2016

Unsure about which part being in core? Just the tool or the metadata? Are
you giving a thumbs down to the entire idea or just one aspect of it?
On Apr 30, 2016 5:25 PM, "Jeremiah Senkpiel" notifications@github.com
wrote:

I'm really not sure we should be checking this into core; could it be part
of the website tooling instead?


You are receiving this because you are on a team that was mentioned.
Reply to this email directly or view it on GitHub
#6495 (comment)

@jasnell
Copy link
Member

jasnell commented May 1, 2016

I'm definitely +1 on adding the metadata to the docs. I'm agnostic about
where the tool lives as long as it's easy to access and use. Can you expand
a bit more on his this tool is expected to be used, who is expected to be
using it, and where, etc. That would help us determine best where it should
go.
On Apr 30, 2016 5:12 PM, "Anna Henningsen" notifications@github.com wrote:

Checklist

  • tests and code linting passes
  • a test and/or benchmark is included
  • documentation is changed or added … kinda.
  • the commit message follows commit guidelines

Affected core subsystem(s)

tools, doc
Description of change

This is a continuation from #3867

#3867. Original description:

This commit introduces the Documentation YAML metadata concept to the
documentation.

  • Parses added or Added for HTML
  • Parses all data for JSON

This is in prelude to #3713 #3713.

Markup would look like this:

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


Changed/additionally included in this PR:

  • Rebased the original commit to master, and applied fixes so linting
    passes with the current rules.
  • Allow the added: attribute to be a collection of versions rather
    than a single one, since semver-minors can trickle down to previous major
    versions.
  • Add a deprecated: attribute.
  • As an example, add these attributes in buffer.md to show how this
    will work in real life.

/cc @nodejs/documentation

https://github.com/orgs/nodejs/teams/documentation

You can view, comment on, or merge this pull request online at:

#6495
Commit Summary

  • tools: parse documentation metadata
  • tools: check for yaml when generating json output
  • test,tools: test yaml parsing of doctool
  • tools: allow multiple added: version entries
  • doc: add added: information for buffer

File Changes

Patch Links:


You are receiving this because you are on a team that was mentioned.
Reply to this email directly or view it on GitHub
#6495

@addaleax
Copy link
Member Author

addaleax commented May 1, 2016

Just to be clear, this is not adding anything radically new to core. It’s adding a feature to the already existing doctool; and as far as that is concerned, I’d be +1 on moving that out of the core repo, but that seems like a completely different discussion to me.

  • Who is using this? People who write/update docs. Unfortunately, it seems like it doesn’t makes sense to add documentation metadata when landing stuff (unless the version in which that will be included is known for sure at time of landing). But after releases with new features, noting the version in which something was added would be very helpful.
  • Who is using this?, the other side: Everyone who wants to be able to look into the docs and see in which version support for something was added. This was requested multiple times and was pretty much universally received as a good idea. (Show "supported since version x" for functions in API documentation #3713, Add supported Node.js versions in latest Docs #6470, possibly more)
  • Where: The docs for specific things. In its current state, this would only apply for anything with that has its own markdown heading, but people seemed to agree with that approach in the discussion of the original PR. I think the last commit here (adding metadata to buffer.md) is a pretty good example of that.

@addaleax
Copy link
Member Author

addaleax commented May 1, 2016

Oh, and if you’re talking about the diff size: I get that. Not so excited about it either, but that’s mostly dependencies and people were fine with it in the original PR, so I just went with it.

@jasnell
Copy link
Member

jasnell commented May 1, 2016

Thank you @addaleax! I've never been that happy with all the node_modules we check into the repo (for eslint, doc, npm, etc). I'd love to find an effective way of moving those out.

@addaleax addaleax added the wip Issues and PRs that are still a work in progress. label May 1, 2016
@addaleax
Copy link
Member Author

addaleax commented May 1, 2016

Very much +1 to that, @jasnell. I mean, eslint brings an impressive 12MB copy of lodash into the repo, and this PR in its current state would add another (older, therefore smaller) copy.

I think having node_modules/ for npm is unavoidable without having some sort of chicken-egg-problem, but how about installing the deps from make? Would love to hear other suggestions, though.

@Trott
Copy link
Member

Trott commented May 1, 2016

@addaleax If you haven't done so already, you can use https://www.npmjs.com/package/dmn to slightly reduce the number of files in node_modules. (I think @targos initially pointed that package out to me.)

@Fishrock123
Copy link
Contributor

Unsure about which part being in core? Just the tool or the metadata? Are
you giving a thumbs down to the entire idea or just one aspect of it?

The metadata is fine, it's hidden in an html comment anyways.

The toolchain is large & will likely change significantly over time, imo. Also, this isn't as critical as the base docs parsing, nor our bundling of npm and eslint.

@addaleax
Copy link
Member Author

addaleax commented May 1, 2016

@Trott Thanks for the pointer… that works, but it doesn’t seem to help a lot.

I’ll try to edit the first commit with updated dependency versions, that seems to help much more.

@addaleax addaleax force-pushed the doc-parse-since-ver2 branch 2 times, most recently from cc43f2a to 2715e37 Compare May 1, 2016 14:20
@addaleax
Copy link
Member Author

addaleax commented May 1, 2016

Updated this PR with a hack to load js-yaml from the eslint dependency tree. Definitely not a perfect solution, but it works.

@Fishrock123 Is this something that would address your concerns?

@jasnell
Copy link
Member

jasnell commented May 1, 2016

@addaleax ... personally, I certainly wouldn't mind having to do a make install-tools or some such but that's just me. It would be relatively painless. However, it would add an additional requirement to get started and would complicate CI so I'm not sure what the right solution is just yet.

@addaleax
Copy link
Member Author

addaleax commented May 1, 2016

@jasnell Yes, but let’s save that for another PR. ;-)

@Fishrock123
Copy link
Contributor

@jasnell Is this something we actually need to install in CI though?

If we saved this for npm install it should only have to do that for the website? I'm ok with not having this available locally without an install, personally. The version of the docs someone has will still always match the version they are using.

@Fishrock123
Copy link
Contributor

Fishrock123 commented May 1, 2016

Updated this PR with a hack to load js-yaml from the eslint dependency tree. Definitely not a perfect solution, but it works.

@Fishrock123 Is this something that would address your concerns?

Not really, that's actually more concerning imo. I think we should explore having an npm install for this additional info. :)

edit: p.s. I'm in favor of this change but really would like to not be checking in even more npm module to the git source.

@addaleax
Copy link
Member Author

addaleax commented May 1, 2016

Is this something we actually need to install in CI though?

The doctool is being tested in CI, as far as I can tell. But if you want this whole thing optional anyway, then the tests would probably have to detect the availability, too.

I'm in favor of this change but really would like to not be checking in even more npm module to the git source.

Just something to keep in mind: Adding copies of files does not increase the repo size, and anything that would be added here is already present in the repo.

@jasnell
Copy link
Member

jasnell commented May 1, 2016

@Fishrock123 ... I was referring to all of the tools that have node_modules, eslint included.

@Fishrock123
Copy link
Contributor

Just something to keep in mind: Adding copies of files does not increase the repo size, and anything that would be added here is already present in the repo.

Wait, what else do we use it for here?

Taking a second look, if that's all that's needed this isn't a big deal, I thought this brought in lots of modules...?

@addaleax
Copy link
Member Author

addaleax commented May 2, 2016

@Fishrock123 This uses js-yaml, and eslint already has that checked in. It’s the only added top-level dependency here, the rest are transitive dependencies. It’s why I proposed the current solution of hackily loading that from the eslint tree instead – I would be okay with that too, until the doctool gets separated out into its own repo or something.

@eljefedelrodeodeljefe
Copy link
Contributor

nice hack. If that works we should have it. LGTM

@addaleax addaleax removed the wip Issues and PRs that are still a work in progress. label May 2, 2016
@jasnell
Copy link
Member

jasnell commented May 3, 2016

Generally LGTM, running local appears to do exactly what it needs to do :-)
Let's just make sure we follow up on whether or not we can simplify the node_modules situation.

@addaleax
Copy link
Member Author

addaleax commented May 3, 2016

@Fishrock123 @Trott Are you okay with the current node_modules/ situation in this PR, at least as a temporary solution?

addaleax added a commit to addaleax/node that referenced this pull request Jul 12, 2016
Add `added:` and `deprecated:` entries to buffer.md.
These are incomplete (particularly for some of the ancient features),
but correct to the best of my knowledge. This serves as a
demonstration of how the `added:`/`deprecated:` metadata may be
implemented in 'real' docs.

PR-URL: nodejs#6495
Reviewed-By: Robert Jefe Lindstaedt <robert.lindstaedt@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
Add `added:` and `deprecated:` entries to buffer.md.
These are incomplete (particularly for some of the ancient features),
but correct to the best of my knowledge. This serves as a
demonstration of how the `added:`/`deprecated:` metadata may be
implemented in 'real' docs.

PR-URL: #6495
Reviewed-By: Robert Jefe Lindstaedt <robert.lindstaedt@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
addaleax added a commit to addaleax/node that referenced this pull request Jul 12, 2016
Add a hack js-yaml module to the doctool dependencies that simply
loads the one that’s included with eslint.

This helps avoiding to check in the whole dependency tree into
the core repo.

PR-URL: nodejs#6495
Reviewed-By: Robert Jefe Lindstaedt <robert.lindstaedt@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
addaleax pushed a commit to addaleax/node that referenced this pull request Jul 12, 2016
This commit introduces the Documentation YAML metadata concept to the
documentation.

- Parses added or Added for HTML
- Parses all data for JSON

Ref: nodejs#3867
Ref: nodejs#3713
Ref: nodejs#6470
PR-URL: nodejs#6495
Reviewed-By: Robert Jefe Lindstaedt <robert.lindstaedt@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
addaleax added a commit to addaleax/node that referenced this pull request Jul 12, 2016
Add checks that make sure the doctool parses metadata correctly.

PR-URL: nodejs#6495
Reviewed-By: Robert Jefe Lindstaedt <robert.lindstaedt@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
addaleax added a commit to addaleax/node that referenced this pull request Jul 12, 2016
Allow multiple `added:` version entries, since semver-minors
can trickle down to previous major versions, and thus
features may have been added in multiple versions.

Also include `deprecated:` entries and apply the same logic
to them for consistency.

Stylize the added HTML as `Added in:` and `Deprecated since:`.

PR-URL: nodejs#6495
Reviewed-By: Robert Jefe Lindstaedt <robert.lindstaedt@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
addaleax added a commit to addaleax/node that referenced this pull request Jul 12, 2016
Skip the doctool tests when js-yaml, which is currently `require()`d
from the eslint source tree, is missing. This can happen, for example,
because eslint is not included in the release source tarballs.

Fixes: nodejs#7201
Ref: nodejs#6495
PR-URL: nodejs#7218
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
Skip the doctool tests when js-yaml, which is currently `require()`d
from the eslint source tree, is missing. This can happen, for example,
because eslint is not included in the release source tarballs.

Fixes: #7201
Ref: #6495
PR-URL: #7218
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
Add `added:` and `deprecated:` entries to buffer.md.
These are incomplete (particularly for some of the ancient features),
but correct to the best of my knowledge. This serves as a
demonstration of how the `added:`/`deprecated:` metadata may be
implemented in 'real' docs.

PR-URL: #6495
Reviewed-By: Robert Jefe Lindstaedt <robert.lindstaedt@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
Add a hack js-yaml module to the doctool dependencies that simply
loads the one that’s included with eslint.

This helps avoiding to check in the whole dependency tree into
the core repo.

PR-URL: #6495
Reviewed-By: Robert Jefe Lindstaedt <robert.lindstaedt@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
This commit introduces the Documentation YAML metadata concept to the
documentation.

- Parses added or Added for HTML
- Parses all data for JSON

Ref: #3867
Ref: #3713
Ref: #6470
PR-URL: #6495
Reviewed-By: Robert Jefe Lindstaedt <robert.lindstaedt@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
Add checks that make sure the doctool parses metadata correctly.

PR-URL: #6495
Reviewed-By: Robert Jefe Lindstaedt <robert.lindstaedt@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
Allow multiple `added:` version entries, since semver-minors
can trickle down to previous major versions, and thus
features may have been added in multiple versions.

Also include `deprecated:` entries and apply the same logic
to them for consistency.

Stylize the added HTML as `Added in:` and `Deprecated since:`.

PR-URL: #6495
Reviewed-By: Robert Jefe Lindstaedt <robert.lindstaedt@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
Skip the doctool tests when js-yaml, which is currently `require()`d
from the eslint source tree, is missing. This can happen, for example,
because eslint is not included in the release source tarballs.

Fixes: #7201
Ref: #6495
PR-URL: #7218
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jul 12, 2016
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
Add `added:` and `deprecated:` entries to buffer.md.
These are incomplete (particularly for some of the ancient features),
but correct to the best of my knowledge. This serves as a
demonstration of how the `added:`/`deprecated:` metadata may be
implemented in 'real' docs.

PR-URL: #6495
Reviewed-By: Robert Jefe Lindstaedt <robert.lindstaedt@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
Add a hack js-yaml module to the doctool dependencies that simply
loads the one that’s included with eslint.

This helps avoiding to check in the whole dependency tree into
the core repo.

PR-URL: #6495
Reviewed-By: Robert Jefe Lindstaedt <robert.lindstaedt@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
This commit introduces the Documentation YAML metadata concept to the
documentation.

- Parses added or Added for HTML
- Parses all data for JSON

Ref: #3867
Ref: #3713
Ref: #6470
PR-URL: #6495
Reviewed-By: Robert Jefe Lindstaedt <robert.lindstaedt@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
Add checks that make sure the doctool parses metadata correctly.

PR-URL: #6495
Reviewed-By: Robert Jefe Lindstaedt <robert.lindstaedt@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
Allow multiple `added:` version entries, since semver-minors
can trickle down to previous major versions, and thus
features may have been added in multiple versions.

Also include `deprecated:` entries and apply the same logic
to them for consistency.

Stylize the added HTML as `Added in:` and `Deprecated since:`.

PR-URL: #6495
Reviewed-By: Robert Jefe Lindstaedt <robert.lindstaedt@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
Skip the doctool tests when js-yaml, which is currently `require()`d
from the eslint source tree, is missing. This can happen, for example,
because eslint is not included in the release source tarballs.

Fixes: #7201
Ref: #6495
PR-URL: #7218
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
Add `added:` and `deprecated:` entries to buffer.md.
These are incomplete (particularly for some of the ancient features),
but correct to the best of my knowledge. This serves as a
demonstration of how the `added:`/`deprecated:` metadata may be
implemented in 'real' docs.

PR-URL: #6495
Reviewed-By: Robert Jefe Lindstaedt <robert.lindstaedt@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
Add a hack js-yaml module to the doctool dependencies that simply
loads the one that’s included with eslint.

This helps avoiding to check in the whole dependency tree into
the core repo.

PR-URL: #6495
Reviewed-By: Robert Jefe Lindstaedt <robert.lindstaedt@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
This commit introduces the Documentation YAML metadata concept to the
documentation.

- Parses added or Added for HTML
- Parses all data for JSON

Ref: #3867
Ref: #3713
Ref: #6470
PR-URL: #6495
Reviewed-By: Robert Jefe Lindstaedt <robert.lindstaedt@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
Add checks that make sure the doctool parses metadata correctly.

PR-URL: #6495
Reviewed-By: Robert Jefe Lindstaedt <robert.lindstaedt@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
Allow multiple `added:` version entries, since semver-minors
can trickle down to previous major versions, and thus
features may have been added in multiple versions.

Also include `deprecated:` entries and apply the same logic
to them for consistency.

Stylize the added HTML as `Added in:` and `Deprecated since:`.

PR-URL: #6495
Reviewed-By: Robert Jefe Lindstaedt <robert.lindstaedt@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
Skip the doctool tests when js-yaml, which is currently `require()`d
from the eslint source tree, is missing. This can happen, for example,
because eslint is not included in the release source tarballs.

Fixes: #7201
Ref: #6495
PR-URL: #7218
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
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.

7 participants