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.

Sign up for GitHub

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

Jump to bottom

Fixing jsdoc parsing of functions that are defined over multiple lines (Fixes #410) #360

Closed
wants to merge 4 commits into from
Closed

Fixing jsdoc parsing of functions that are defined over multiple lines (Fixes #410) #360

wants to merge 4 commits into from

Conversation

pjjanak
Copy link

@pjjanak pjjanak commented Mar 27, 2015

As it was implemented, the jsdoc parser would look only the first non-blank line immediately preceding a function declaration. However, the line that was set as the beginning of a function declaration was where the opening bracket ({) was. This is insufficient for functions whose definitions span multiple lines. For example, this declaration would not find the comments above it:

/**
 * Clones rows
 **/
cloneWithRows(
       dataBlob: Array<any> | {[key: string]: any},
       rowIdentities: ?Array<string>
   ): ListViewDataSource {
...
}

With this change, the parser will first check if we have a closing parenthesis. If we do and don't have a matching open parenthesis we continue moving up the lines until we find it. Then we set previous line to be the line before that, the true beginning of the function declaration.

@pjjanak
Copy link
Author

pjjanak commented Mar 27, 2015

Forgot to make a branch and ended up adding in some extra docs to this PR too...however their getting rendered depends on the original commit (03a562f). Just an FYI about the second and third commits.

@pjjanak
Copy link
Author

pjjanak commented Mar 27, 2015

Also, I did complete the CLA.

vjeux
vjeux reviewed Mar 27, 2015
View reviewed changes
website/jsdocs/jsdocs.js
@@ -131,6 +131,16 @@ function getDocBlock(node, commentsForFile, linesForFile) {
}
var docblock;
var prevLine = node.loc.start.line - 1;
var startText = linesForFile[prevLine].trim();

// Get to actual start of function declaration (for multi-line declarations)
Copy link
Contributor

Choose a reason for hiding this comment

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

@hansonw can you review this part?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch @pjjanak! However I don't think this will work; for example there are Flow typehints that contain left parens (namely for function types).

I did some digging, and actually this is only an issue for multi-line class method declarations. It looks like Esprima handles ES6 class methods by creating a MethodDefinition AST node which contains another AST node with the actual FunctionDeclaration - the MethodDefinition has the correct line range.

A simpler fix would be to just call getDocBlock on the MethodDefinition AST node in getClassData near line 395 below, and override the docblock there.

@hansonw
Copy link
Contributor

hansonw commented Mar 28, 2015

Looks good to me! Leaving the rest to you, @vjeux.

@pjjanak pjjanak changed the title Fixing jsdoc parsing of functions that are defined over multiple lines Fixing jsdoc parsing of functions that are defined over multiple lines (Fixes #410) Mar 29, 2015
@pjjanak pjjanak mentioned this pull request Mar 29, 2015
Closed
@vjeux vjeux closed this Apr 3, 2015
@vjeux vjeux reopened this Apr 3, 2015
@vjeux
Copy link
Contributor

vjeux commented Apr 6, 2015

Thanks, it is now in the repo!

@vjeux vjeux closed this Apr 6, 2015
vjeux pushed a commit to vjeux/react-native that referenced this pull request Apr 13, 2015
Fixing jsdoc parsing of functions that are defined over multiple lines (
7f12efa 
Fixes facebook#410)

Summary:
As it was implemented, the jsdoc parser would look only the first non-blank line immediately preceding a function declaration. However, the line that was set as the beginning of a function declaration was where the opening bracket (`{`) was. This is insufficient for functions whose definitions span multiple lines. For example, this declaration would not find the comments above it:

```
/**
 * Clones rows
 **/
cloneWithRows(
       dataBlob: Array<any> | {[key: string]: any},
       rowIdentities: ?Array<string>
   ): ListViewDataSource {
...
}
```

With this change, the parser will first check if we have a closing parenthesis. If we do and don't have a matching open parenthesis we continue moving up the lines until we find it. Then we set previous line to be the line before that, the true beginning of the function declaration.
Closes facebook#360
Github Author: Peter Janak <pjanak@nhl.com>

Test Plan: Run the website
vjeux pushed a commit to vjeux/react-native that referenced this pull request Apr 14, 2015
Fixing jsdoc parsing of functions that are defined over multiple lines (
e239feb 
Fixes facebook#410)

Summary:
As it was implemented, the jsdoc parser would look only the first non-blank line immediately preceding a function declaration. However, the line that was set as the beginning of a function declaration was where the opening bracket (`{`) was. This is insufficient for functions whose definitions span multiple lines. For example, this declaration would not find the comments above it:

```
/**
 * Clones rows
 **/
cloneWithRows(
       dataBlob: Array<any> | {[key: string]: any},
       rowIdentities: ?Array<string>
   ): ListViewDataSource {
...
}
```

With this change, the parser will first check if we have a closing parenthesis. If we do and don't have a matching open parenthesis we continue moving up the lines until we find it. Then we set previous line to be the line before that, the true beginning of the function declaration.
Closes facebook#360
Github Author: Peter Janak <pjanak@nhl.com>

Test Plan: Run the website
vjeux pushed a commit to vjeux/react-native that referenced this pull request Apr 15, 2015
Fixing jsdoc parsing of functions that are defined over multiple lines (
de8a370 
Fixes facebook#410)

Summary:
As it was implemented, the jsdoc parser would look only the first non-blank line immediately preceding a function declaration. However, the line that was set as the beginning of a function declaration was where the opening bracket (`{`) was. This is insufficient for functions whose definitions span multiple lines. For example, this declaration would not find the comments above it:

```
/**
 * Clones rows
 **/
cloneWithRows(
       dataBlob: Array<any> | {[key: string]: any},
       rowIdentities: ?Array<string>
   ): ListViewDataSource {
...
}
```

With this change, the parser will first check if we have a closing parenthesis. If we do and don't have a matching open parenthesis we continue moving up the lines until we find it. Then we set previous line to be the line before that, the true beginning of the function declaration.
Closes facebook#360
Github Author: Peter Janak <pjanak@nhl.com>

Test Plan: Run the website
jfrolich pushed a commit to jfrolich/react-native that referenced this pull request Apr 22, 2020
@MoOx
Fix CI for jsx 3 (facebook#360)
97bcaee 
* Fix CI

* Update config.yml
acoates-ms added a commit to acoates-ms/react-native that referenced this pull request Jun 1, 2020
@acoates-ms
Get 0.60-stable publishing (facebook#360)
4ae50a6
ayushjainrksh referenced this pull request in MLH-Fellowship/react-native Jul 2, 2020
@allanxp4 @charpeni
Windows and Linux shortcuts for developer menu (#360)
c09960f 
Added shortcuts for developer menu on Windows and Linux. Typing adb commands is not necessary.
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
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pjjanak @hansonw @vjeux