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

The "advanced" api-demo scenario does not collect all TypeScript comments #56

Closed
octogonz opened this issue Aug 31, 2018 · 7 comments
Closed
Labels
effort: easy Probably a quick fix. Want to contribute? :-) help wanted If you're looking to contribute, this issue is a good place to start!

Comments

@octogonz
Copy link
Collaborator

@Gerrit0 commented in #55 (comment) that the walkCompilerAstAndFindComments() function does not find some comments.

We should find a better example of how to do this. @eps1lon suggested:

You could use the checker to get the symbol of a node. Symbols have a method getDocumentationComment. Never used this method. The name just sounds fitting.

@sandersn / @DanielRosenwasser could you provide some guidance?

@sandersn
Copy link
Member

sandersn commented Sep 1, 2018

Quick on-the-bus question: have you tried ts.getJSDocTags(node: Node)? That only returns jsdoc of course, and it chops everything up into tags, but it might be enough.

I’ll take a closer look when I’m back in the office after the holiday.

@octogonz octogonz added help wanted If you're looking to contribute, this issue is a good place to start! effort: easy Probably a quick fix. Want to contribute? :-) labels Sep 1, 2018
@octogonz
Copy link
Collaborator Author

octogonz commented Sep 1, 2018

@sandersn I don't think ts.getJSDocTags(node: Node) will work. It attempts to parse the code comment (using the compiler's grammar rules which don't yet support TSDoc). Whereas for the purposes of this demo, we want to extract the /** ... */ text range so that we can pass it to the TSDoc library's parser.

@Gerrit0
Copy link
Contributor

Gerrit0 commented Sep 4, 2018

I've been looking into this a bit more and noticed the ts.getCommentRange method, which might meet the requirements.

@octogonz
Copy link
Collaborator Author

octogonz commented Sep 4, 2018

I tried ts.getCommentRange() but it returns a nonempty range for most AST nodes, which includes code expressions. The docs suggest that this function is meant to be used when injecting comments in emitted output.

So we still don't have an answer: What's a good way to scan a source file and find all the /** */ comment ranges? @RyanCavanaugh

@octogonz
Copy link
Collaborator Author

octogonz commented Sep 4, 2018

@eps1lon thanks for contributing this PR. Today I'm going to look at the various options and see what's the best approach for this demo. I agree with @Gerrit0 that the demo itself should be self-contained, but maybe tsutils will provide a useful model.

@octogonz
Copy link
Collaborator Author

octogonz commented Sep 5, 2018

After debugging into the compiler library, I realized that advancedDemo.ts#L69 was calling SourceFile.getText() instead of SourceFile.getFullText(). This sometimes some leading comments/whitespace, which shifted the buffer indexes around, producing mostly correct but sometimes very bizarre results. After fixing that, things got a lot more sane.

Although tsutils.forEachComment() is a pretty good way to enumerate all comments associated with a subtree, I'm not sure it's best for this demo. The demo is meant to model a tool that discovers declarations and then parses the TSDoc that would conventionally be associated with a declaration. We probably would not want to exhaustively consider every comment in the input. Also we should ignore // and /* */ style comments. The compiler's internal ts.getJSDocCommentRanges() seems like a closer match for what we need.

However, with this approach we sometimes pickup duplicates when iterating the AST tree. For example when analyzing "public static example(): void {" and calling ts.getJSDocCommentRanges(), it will find the same comment for both the PublicKeyword node and the MethodDeclaration node (since they have the same physical position). We need to filter out the "insignificant" nodes. @RyanCavanaugh pointed me to ts.isDeclaration() which should work, however that is an unfortunately internal API.

I'll see if I can combine all these ideas together into a PR.

@octogonz
Copy link
Collaborator Author

This was fixed in PR #59.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort: easy Probably a quick fix. Want to contribute? :-) help wanted If you're looking to contribute, this issue is a good place to start!
Projects
None yet
Development

No branches or pull requests

3 participants