Skip to content

Conversation

@esmacik
Copy link
Contributor

@esmacik esmacik commented Aug 12, 2019

This is the first iteration of my solution to linking to external library types for the Firebase Functions Reference documentation.

  1. Map external types to their links.
  2. Create a DOM of each HTML file.
  3. When fixing links, intersect HTML data of generated doc and query tags of type tsd-signature-type.
  4. Add link to matching types

In these staged files, there are now links to UserInfo, UserRecord, and DocumentSnapshot types:

@esmacik esmacik requested a review from thechenky as a code owner August 12, 2019 22:34
@esmacik esmacik requested a review from hiranya911 August 12, 2019 22:35
@esmacik esmacik requested a review from egilmorez August 13, 2019 19:53
Copy link

@hiranya911 hiranya911 left a comment

Choose a reason for hiding this comment

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

Looks mostly good. Just a few suggestions to improve on.

@egilmorez egilmorez changed the title Linking to extrernal types - docgen Linking to external types - docgen Aug 13, 2019
…console log, JSDOM is now imported with one line, and Map of types and links is now created from a JSON file, Still need to resolve a couple of Hiranya's comments.
…' rather than 'read file'. JSON file is now simpler. Script only replaces fully qualified names rather than aliases in document.
*/
function addTypeAliasLinks(data) {
const htmlDom = new JSDOM(data);
const fileTags = htmlDom.window.document.querySelectorAll(".tsd-signature-type");
Copy link
Contributor

Choose a reason for hiding this comment

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

Not immediately clear why we're selecting .tsd-signature-type?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would add a comment here explaining why this tag, so that the next contributor knows the context.

Copy link
Contributor

Choose a reason for hiding this comment

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

Adding what I think Erik was doing -- just picking an ID to iterate through that would always be associated with potential external types in TypeDoc.

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

1 similar comment
@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@egilmorez egilmorez requested a review from kevinajian October 23, 2019 22:21
@egilmorez egilmorez requested a review from joehan October 24, 2019 17:35
Copy link
Contributor

@joehan joehan left a comment

Choose a reason for hiding this comment

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

Code looks fine to me, and after looking thru the staging site, the choice of .tsd-signature-type seems appropriate. However, in the description, it says that DocumentSnapshot should now be hyperlinked on the staged site, and I don't see that. Has this code just been unstaged in the past few months?
Screen Shot 2019-10-24 at 10 35 20 AM

@egilmorez egilmorez dismissed hiranya911’s stale review October 24, 2019 20:00

Checked with Hiranya offline; all has been address afaict.

@joehan
Copy link
Contributor

joehan commented Oct 24, 2019

@googlebot I fixed it.

@googlebot
Copy link

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

1 similar comment
@googlebot
Copy link

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

@joehan
Copy link
Contributor

joehan commented Oct 24, 2019

Overriding the CLA bot here, as we have a CLA from the commit author from a different email, and the commits causing the check to fail are from github.

@egilmorez egilmorez merged commit c5d3a7b into master Oct 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants