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

Better support for typedef @property #135

Merged
merged 4 commits into from
Jun 3, 2020
Merged

Better support for typedef @property #135

merged 4 commits into from
Jun 3, 2020

Conversation

mramato
Copy link
Contributor

@mramato mramato commented Jun 1, 2020

Given the following JSDoc

/**
 * @typedef {Object} BillboardGraphics.ConstructorOptions
 *
 * Initialization options for the BillboardGraphics constructor
 *
 * @property {Property | boolean} [show=true] A boolean Property specifying the visibility of the billboard.
 */

tsd-jsdoc was producing the following TS code

/**
 * Initialization options for the BillboardGraphics constructor
 * @property [show = true] - A boolean Property specifying the visibility of the billboard.
 */
type ConstructorOptions = {
    show?: Property | boolean;
};

@property is not valid TS markup and the actual property, show, does not include its description or default value.

After this change the same JSDoc markup produces much nicer code that handles both of these:

/**
 * Initialization options for the BillboardGraphics constructor
 */
type ConstructorOptions = {
    /**
     * A boolean property specifying the visibility of the billboard.
     * @defaultValue true
     */
    show?: Property | boolean;
};

I updated the tests, to match the new output and manually verified its what I would expect.

@mramato
Better support for typedef @Property
77ffb34 
Given the following JSDoc

```js
/**
 * @typedef {Object} BillboardGraphics.ConstructorOptions
 *
 * Initialization options for the BillboardGraphics constructor
 *
 * @Property {Property | boolean} [show=true] A boolean Property specifying the visibility of the billboard.
 */
```

tsd-jsdoc was producing the following TS code

```ts
/**
 * Initialization options for the BillboardGraphics constructor
 * @Property [show = true] - A boolean Property specifying the visibility of the billboard.
 */
type ConstructorOptions = {
    show?: Property | boolean;
};
```

`@property` is not valid TS markup and the actual property, show, does
not include it's description or default value.

After this change the same JSDoc markup produces much nicer code that
handles both of these:

```ts
/**
 * Initialization options for the BillboardGraphics constructor
 */
type ConstructorOptions = {
    /**
     * A boolean property specifying the visibility of the billboard.
     * @DefaultValue true
     */
    show?: Property | boolean;
};
```

I updated the tests, to match the new output and manually verified its
what I would expect.
mramato
mramato commented Jun 1, 2020
View reviewed changes
src/type_resolve_helpers.ts Outdated

// !parent ensures we are dealing with a top-level typedef.
// So that the tsd-doc is added at the property level.
if(!parent && (node.prop.description || node.prop.defaultvalue)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@englercj Is there a better way I should be checking this? The other option is threading the doclet through the calls so that I have access to doclet.kind here, which should be trivial to do but didn't want to make a bigger change before checking with you.

Copy link
Owner

Choose a reason for hiding this comment

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

I think this check is reasonable

@mramato mramato mentioned this pull request Jun 1, 2020
Merged
12 tasks
englercj
englercj requested changes Jun 3, 2020
View reviewed changes
Copy link
Owner

@englercj englercj left a comment

Choose a reason for hiding this comment

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

Looks good, just had some minor style suggestions.

src/type_resolve_helpers.ts Outdated Show resolved Hide resolved
src/type_resolve_helpers.ts Outdated Show resolved Hide resolved
mramato and others added 2 commits June 3, 2020 10:00
@mramato @englercj
Update src/type_resolve_helpers.ts
7ab662e 
Co-authored-by: Chad Engler <englercj@live.com>
@mramato @englercj
Update src/type_resolve_helpers.ts
e0bdaaa 
Co-authored-by: Chad Engler <englercj@live.com>
@mramato
Copy link
Contributor Author

mramato commented Jun 3, 2020

Thanks @englercj, I accepted your changes so I think this is ready.

englercj
englercj reviewed Jun 3, 2020
View reviewed changes
src/create_helpers.ts Outdated Show resolved Hide resolved
@englercj englercj merged commit 3c6e2db into englercj:master Jun 3, 2020
@mramato mramato deleted the typedef-properties branch June 3, 2020 14:14
@mramato
Copy link
Contributor Author

mramato commented Jun 3, 2020

Thanks @englercj! Any idea when tsd-jsdoc will get an updated npm release?

@mramato
Copy link
Contributor Author

mramato commented Jun 5, 2020

@englercj Are there any plans to do a new release anytime soon? I would like to make use of this PR and I didn't want to fork/publish to a different package just to use it.

I know you are busy and I'm still a complete stranger, but long term I have no problem helping out with maintaining this repository once I earn your trust, since CesiumJS is not dependent on it. Thanks!

@englercj
Copy link
Owner

englercj commented Jun 13, 2020 •
edited
Loading

There are some bugs and regressions that came up recently. I'm waiting for those to get fixed first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@englercj englercj englercj requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mramato @englercj