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

refactor(nest): Better nesting implementation #732

Merged
merged 7 commits into from Apr 21, 2017

Conversation

@tmcw
Copy link
Member

tmcw commented Apr 15, 2017

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 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

@tmcw tmcw force-pushed the nest-better branch 6 times, most recently from 44371d3 to d5461de Apr 15, 2017
@tmcw tmcw requested review from arv and jfirebaugh Apr 15, 2017
@tmcw tmcw force-pushed the nest-better branch 2 times, most recently from dcf7201 to ad5c22e Apr 15, 2017
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
arv approved these changes Apr 17, 2017
Copy link
Contributor

arv left a comment

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.

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

declare type CommentTag = {
declare type CommentTagBase = {

This comment has been minimized.

Copy link
@arv

arv Apr 17, 2017

Contributor

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

type CommentTagBase = {
...

This comment has been minimized.

Copy link
@tmcw

tmcw Apr 17, 2017

Author Member

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.

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

This comment has been minimized.

Copy link
@arv

arv Apr 17, 2017

Contributor

Looks like a constant to me

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

This comment has been minimized.

Copy link
@arv

arv Apr 17, 2017

Contributor

Does this handle object destructuring shorthand?

function f({x}) {

This comment has been minimized.

Copy link
@arv

arv Apr 17, 2017

Contributor

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

lib/nest.js Outdated

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

function rejectUnnamedTags(

This comment has been minimized.

Copy link
@arv

arv Apr 17, 2017

Contributor

reject is a bit misleading... maybe remove?

],
'renaming'
);

This comment has been minimized.

Copy link
@arv

arv Apr 17, 2017

Contributor

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

This comment has been minimized.

Copy link
@tmcw

tmcw Apr 17, 2017

Author Member

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

@coveralls

This comment has been minimized.

Copy link

coveralls commented Apr 17, 2017

Coverage Status

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

@tmcw

This comment has been minimized.

Copy link
Member Author

tmcw commented Apr 17, 2017

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 :)

@tmcw tmcw removed the request for review from jfirebaugh Apr 17, 2017
@coveralls

This comment has been minimized.

Copy link

coveralls commented Apr 17, 2017

Coverage Status

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

@coveralls

This comment has been minimized.

Copy link

coveralls commented Apr 17, 2017

Coverage Status

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

@coveralls

This comment has been minimized.

Copy link

coveralls commented Apr 17, 2017

Coverage Status

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

@tmcw

This comment has been minimized.

Copy link
Member Author

tmcw commented Apr 17, 2017

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.

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

This comment has been minimized.

Copy link
@jfirebaugh

jfirebaugh Apr 17, 2017

Member

Doesn't nest need to come after inferMembership?

This comment has been minimized.

Copy link
@tmcw

tmcw Apr 19, 2017

Author Member

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

This comment has been minimized.

Copy link

coveralls commented Apr 19, 2017

Coverage Status

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

@coveralls

This comment has been minimized.

Copy link

coveralls commented Apr 21, 2017

Coverage Status

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

@tmcw tmcw merged commit 7374730 into master Apr 21, 2017
1 of 2 checks passed
1 of 2 checks passed
coverage/coveralls Coverage decreased (-0.4%) to 96.464%
Details
ci/circleci Your tests passed on CircleCI!
Details
@tmcw tmcw deleted the nest-better branch Apr 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.