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: validate apidoc links #21889

Closed
wants to merge 5 commits into from
Closed

Conversation

rubys
Copy link
Member

@rubys rubys commented Jul 19, 2018

When an API signature changes, it may break a link made to it in another part of the documentation. As a part of the generation of all.html, check all such links to make sure that they have a valid target.

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

@@ -73,3 +73,13 @@ all = all.slice(0, apiStart.index + apiStart[0].length) +

// Write results.
fs.writeFileSync(source + '/all.html', all, 'utf8');

// Validate all hrefs have a target
const ids = {};
Copy link
Member

Choose a reason for hiding this comment

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

Can we use a Set for this? I think that might be a tad more natural 🙂

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM, but agreed on using a Set.

@addaleax addaleax added doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory. labels Jul 19, 2018
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt left a comment

Choose a reason for hiding this comment

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

Thank you! Glad to see this automated.

@@ -73,3 +73,13 @@ all = all.slice(0, apiStart.index + apiStart[0].length) +

// Write results.
fs.writeFileSync(source + '/all.html', all, 'utf8');

// Validate all hrefs have a target
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: a period in the end.

@vsemozhetbyt

This comment has been minimized.

@vsemozhetbyt
Copy link
Contributor

Ignorable nit: not sure if this replace() approach is a bit hacky and may be confusing. Maybe a while + exec() way is more straightforward? (Though replace() way may be a common idiom I am not aware of.)

@bnb
Copy link
Contributor

bnb commented Jul 19, 2018

Just wanted to say thank you – this is super awesome ❤️


const href_re = / href="#(\w+)"/g;
while (match = href_re.exec(all)) {
if (!ids.has(match[1])) throw new Error(`link not found: ${match[1]}`);

This comment was marked as resolved.

Copy link
Member Author

@rubys rubys Jul 19, 2018

Choose a reason for hiding this comment

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

I don't think efficiency is a concern here... it will only be retrieved twice at most one time per run, and only if an error is thrown.

@vsemozhetbyt

This comment has been minimized.


// Validate all hrefs have a target.
const ids = new Set();
const id_re = / id="(\w+)"/g;
Copy link
Member

Choose a reason for hiding this comment

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

Nit: can we use camel case?

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt pushed a commit that referenced this pull request Jul 22, 2018
PR-URL: #21889
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@vsemozhetbyt
Copy link
Contributor

Landed in 78039b5
Thank you!

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