refactor(nest): Better nesting implementation #732

tmcw · 2017-04-15T05:16:43Z

Okay, this passes tests and now makes sense! This is a relatively big change, encompassing:

Finally using a proper tree datastructure for nesting parameters and properties
Throwing errors when people reference inferred destructured parameters
Improved inference for array and object destructuring - they are now documented as Array and Object types, rather than any.

I'd greatly appreciate a code review if anyone can spare the time!

TODO

Alignment with specified properties step
This now relies on Permitting numeric keys in parameter names eslint/doctrine#192 to avoid a regression: without a fix or workaround in doclet parsing, it will regress the ability to explicitly document destructuring arrays. Use our temporary fork repo to avoid this being a blocker.

Fixes #554 and fixes #677

@param

This nesting implementation uses a proper recursive tree algorithm Fixes #554 BREAKING CHANGE: referencing inferred destructure params without renaming them, like $0.x, from JSDoc comments will no longer work. To reference them, instead add a param tag to name the destructuring param, and then refer to members of that name. Before: ```js /** * @param {number} $0.x a member of x */ function a({ x }) {} ``` After: ```js /** * @param {Object} options * @param {number} options.x a member of x */ function a({ x }) {} ```

arv

This code is not too easy to follow so I looked closer at the tests and they look good to me.

One way to make reviewing large PRs easier might be to get the formatting changes done in its own commit.

arv · 2017-04-17T00:03:47Z

declarations/comment.js

@@ -58,7 +58,21 @@ declare type CommentContextGitHub = {
  url: string
 };

-declare type CommentTag = {
+declare type CommentTagBase = {


Why do you need to do declare here. Can't you just do:

type CommentTagBase = { ...

Yep! I always wrote declare type because that's what the Flow library definition documentation talks about, but apparently declare actually doesn't mean anything, which is confusing - I'd love to track down whether it has any meaning anywhere, or why it was there in the first place.

arv · 2017-04-17T00:07:38Z

lib/infer/params.js

-function addPrefix(doc, prefix) {
-  if (!Array.isArray(doc) && doc.name) {
-    doc.name = prefix + doc.name;
+// TODO: use a constant


Looks like a constant to me

arv · 2017-04-17T00:10:25Z

lib/infer/params.js

+      return paramToDoc(property, i, prefix + '$' + String(i) + '.');
+    case 'Identifier':
+      // if the destructuring type is an array, the elements
+      // in it are identifiers


Does this handle object destructuring shorthand?

function f({x}) {

Also, isfunction f({x, ...y}) { handled?

arv · 2017-04-17T00:21:55Z

lib/nest.js

+
+const PATH_SPLIT = /(?:\[])?\./g;
+
+function rejectUnnamedTags(


reject is a bit misleading... maybe remove?

arv · 2017-04-17T00:29:03Z

test/lib/infer/params.js

+    ],
+    'renaming'
+  );
+


Maybe add test for function f({x}) {} and function f({x, ...xs}) {}?

👍 added a test for the latter, and it works well. We have a test for the former at L170 of params.js

coveralls · 2017-04-17T01:33:05Z

Coverage decreased (-0.1%) to 96.756% when pulling e160326 on nest-better into d8ec5da on master.

tmcw · 2017-04-17T01:35:37Z

✨ Thank you @arv! Much appreciated!

Re: formatting - something's afoot - since #710 there shouldn't be new surprises with formatting, and I'm seeing some in this issue, specifically in index.js. Digging in, that's because index.js isn't included in the lib/* glob for prettier, which is an oversight on my part: I think the best option is to move index.js to lib/index.js, as is now pretty common in module directory structure. I'll do that in a follow-up PR ( #736 ). Then formatting should be stable and diffs should be nicer :)

coveralls · 2017-04-17T01:48:21Z

Coverage decreased (-0.1%) to 96.756% when pulling 03242f3 on nest-better into d8ec5da on master.

coveralls · 2017-04-17T02:02:23Z

Coverage decreased (-0.1%) to 96.758% when pulling 1f99f9c on nest-better into d8ec5da on master.

coveralls · 2017-04-17T02:19:46Z

Coverage decreased (-0.1%) to 96.76% when pulling e0fa855 on nest-better into d8ec5da on master.

tmcw · 2017-04-17T02:23:49Z

TODO:

/** hi */
function f({ x: { y: { z } } }) {}

Produces

### Table of Contents

-   [f](#f)

## f

hi

**Parameters**

-   `$0` **[Object](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object)**
    -   `$0.x` **[Object](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object)**
        -   `$0.$0.y` **[Object](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object)**
            -   `$0.$0.$0.z`

Which is wrong. I need to do a second pass on the prefixing system.

jfirebaugh · 2017-04-17T22:55:56Z

index.js

    inferParams,
    inferProperties,
    inferReturn,
    inferMembership(),
    inferType,
-    nest,


Doesn't nest need to come after inferMembership?

I don't believe so: nest nests tags (properties, params) - hierarchy nests according to membership. I'm excited to refactor hierarchy too as soon as this is release-ready. But there might be something I'm missing here that isn't uncovered by test fixtures?

…d Array destructuring: documenting destructured array elements with indices instead of names, because the names are purely internal details

coveralls · 2017-04-19T00:23:29Z

Coverage decreased (-0.4%) to 96.462% when pulling af0daf8 on nest-better into d8ec5da on master.

coveralls · 2017-04-21T21:28:10Z

Coverage decreased (-0.4%) to 96.464% when pulling d2ef6a7 on nest-better into d8ec5da on master.

tmcw force-pushed the nest-better branch 6 times, most recently from 44371d3 to d5461de Compare April 15, 2017 19:01

tmcw requested review from arv and jfirebaugh April 15, 2017 19:01

tmcw force-pushed the nest-better branch 2 times, most recently from dcf7201 to ad5c22e Compare April 15, 2017 19:25

tmcw force-pushed the nest-better branch from ad5c22e to 7156122 Compare April 15, 2017 19:33

This was referenced Apr 15, 2017

Is it possible to rename $0 when documenting destructured params? #677

Closed

Implement a proper recursive tree for membership #735

Open

ES6 support #29

Open

arv approved these changes Apr 17, 2017

View reviewed changes

Address review comments

e160326

tmcw removed the request for review from jfirebaugh April 17, 2017 01:36

Reduce testing node requirement back down to 4

03242f3

Don't output empty properties, reduce diff noise

1f99f9c

Rearrange and document params

e0fa855

jfirebaugh reviewed Apr 17, 2017

View reviewed changes

Simplify param inference, update test fixtures. This is focused aroun…

af0daf8

…d Array destructuring: documenting destructured array elements with indices instead of names, because the names are purely internal details

Use temporary fork to get through blocker

d2ef6a7

tmcw merged commit 7374730 into master Apr 21, 2017

tmcw deleted the nest-better branch April 21, 2017 21:28

tmcw mentioned this pull request Apr 22, 2017

Parameter property extensions #726

Closed

tmcw mentioned this pull request Jun 9, 2017

memberof does not cascade down to class members #591

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(nest): Better nesting implementation #732

refactor(nest): Better nesting implementation #732

tmcw commented Apr 15, 2017 •

edited

Loading

arv left a comment

arv Apr 17, 2017

tmcw Apr 17, 2017

arv Apr 17, 2017

arv Apr 17, 2017

arv Apr 17, 2017

arv Apr 17, 2017

arv Apr 17, 2017

tmcw Apr 17, 2017

coveralls commented Apr 17, 2017

tmcw commented Apr 17, 2017

coveralls commented Apr 17, 2017

coveralls commented Apr 17, 2017

coveralls commented Apr 17, 2017

tmcw commented Apr 17, 2017

jfirebaugh Apr 17, 2017

tmcw Apr 19, 2017

coveralls commented Apr 19, 2017

coveralls commented Apr 21, 2017


		const PATH_SPLIT = /(?:\[])?\./g;

		function rejectUnnamedTags(

refactor(nest): Better nesting implementation #732

refactor(nest): Better nesting implementation #732

Conversation

tmcw commented Apr 15, 2017 • edited Loading

TODO

arv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Apr 17, 2017

tmcw commented Apr 17, 2017

coveralls commented Apr 17, 2017

coveralls commented Apr 17, 2017

coveralls commented Apr 17, 2017

tmcw commented Apr 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Apr 19, 2017

coveralls commented Apr 21, 2017

tmcw commented Apr 15, 2017 •

edited

Loading