Skip to content

Add types #3

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
Merged
merged 19 commits into from
Apr 2, 2021
Merged

Add types #3

merged 19 commits into from
Apr 2, 2021

Conversation

guillaumegarcia13
Copy link
Contributor

@guillaumegarcia13 guillaumegarcia13 commented Mar 15, 2021 •
edited
Loading

[Typescript newbie here 👶🏻]
Not too sure about this one... 😬
Would happily be reviewed

ChristianMurphy
ChristianMurphy reviewed Mar 15, 2021
View reviewed changes
Copy link
Member

@ChristianMurphy ChristianMurphy left a comment

Choose a reason for hiding this comment

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

Thanks @guillaumegarcia13!
It would be good to have a type test for this, similar to https://github.com/stefanprobst/xast-util-sitemap/blob/2a1a85a33cbd5a565f4c7fef7f142160a596bd6d/types/test.ts which was added in syntax-tree/xast-util-sitemap#1

@ChristianMurphy ChristianMurphy added ☂️ area/types This affects typings 🦋 type/enhancement This is great to have 🧒 semver/minor This is backwards-compatible change labels Mar 15, 2021
guillaumegarcia13 and others added 6 commits March 30, 2021 21:55
@guillaumegarcia13 @ChristianMurphy
Update package.json
200fc95 
Co-authored-by: Christian Murphy <christian.murphy.42@gmail.com>
@guillaumegarcia13 @ChristianMurphy
Update types/index.d.ts
598c89e 
Co-authored-by: Christian Murphy <christian.murphy.42@gmail.com>
@guillaumegarcia13 @ChristianMurphy
Update types/index.d.ts
43834f1 
Co-authored-by: Christian Murphy <christian.murphy.42@gmail.com>
@guillaumegarcia13
amends based on reviews
8134e93
@guillaumegarcia13
Merge branch 'main' of https://github.com/guillaumegarcia13/unist-uti…
10891a0 
…l-remove into main
@guillaumegarcia13
adds tslint.json
43afc88
@guillaumegarcia13
Copy link
Contributor Author

Hi @ChristianMurphy!
Thank you very much for the guidance.
Hopefully, it's OK now. 😅

ChristianMurphy
ChristianMurphy reviewed Mar 30, 2021
View reviewed changes
Copy link
Member

@ChristianMurphy ChristianMurphy left a comment

Choose a reason for hiding this comment

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

Thanks for following up @guillaumegarcia13! Just a few more tweaks and this should be good to go!

@ChristianMurphy
Copy link
Member

Would it be possible to have type tests for this PR?
Similar to https://github.com/stefanprobst/xast-util-sitemap/blob/2a1a85a33cbd5a565f4c7fef7f142160a596bd6d/types/test.ts which was added in syntax-tree/xast-util-sitemap#1

ChristianMurphy
ChristianMurphy approved these changes Apr 1, 2021
View reviewed changes
ChristianMurphy
ChristianMurphy reviewed Apr 1, 2021
View reviewed changes
guillaumegarcia13 and others added 6 commits April 2, 2021 09:18
@guillaumegarcia13 @ChristianMurphy
Update types/test.ts
027e875 
Co-authored-by: Christian Murphy <christian.murphy.42@gmail.com>
@guillaumegarcia13 @ChristianMurphy
Update types/index.d.ts
7d68fe3 
Co-authored-by: Christian Murphy <christian.murphy.42@gmail.com>
@guillaumegarcia13 @ChristianMurphy
Update types/index.d.ts
931878b 
Co-authored-by: Christian Murphy <christian.murphy.42@gmail.com>
@guillaumegarcia13 @ChristianMurphy
Update package.json
3f226b3 
Co-authored-by: Christian Murphy <christian.murphy.42@gmail.com>
@guillaumegarcia13
adds fixes
8ab150d
@guillaumegarcia13
fixes multiple export with same name
ab3d07f
@wooorm wooorm changed the title adds type definitions Add types Apr 2, 2021
remcohaszing
remcohaszing reviewed Apr 2, 2021
View reviewed changes
types/index.d.ts Outdated
* @param options Whether to drop parent nodes if they had children, but all their children were filtered out. Default is {cascade: true}
* @param test is-compatible test (such as a type)
*/
export function remove<T extends Node>(
Copy link
Member

Choose a reason for hiding this comment

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

This function is exported using module.exports = remove.

To reflect this correctly in the types, this should be exported using:

export = remove

remcohaszing
remcohaszing requested changes Apr 2, 2021
View reviewed changes
@wooorm wooorm merged commit 17f2167 into syntax-tree:main Apr 2, 2021
@wooorm
Copy link
Member

wooorm commented Apr 2, 2021

Thanks all, released in 2.1.0!

@guillaumegarcia13
Copy link
Contributor Author

Thank you all for your patience and advice. 🙏🏻
Sorry for all the "noise" generated by my numerous commits. 🙇🏻‍♂️

@wooorm wooorm added the 💪 phase/solved Post is done label Apr 12, 2021
@remcohaszing remcohaszing mentioned this pull request May 5, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChristianMurphy ChristianMurphy ChristianMurphy approved these changes

@remcohaszing remcohaszing remcohaszing approved these changes

@wooorm wooorm wooorm approved these changes

@BarryThePenguin BarryThePenguin Awaiting requested review from BarryThePenguin

@Murderlon Murderlon Awaiting requested review from Murderlon

Assignees
No one assigned
Labels
☂️ area/types This affects typings 💪 phase/solved Post is done 🧒 semver/minor This is backwards-compatible change 🦋 type/enhancement This is great to have
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@guillaumegarcia13 @ChristianMurphy @wooorm @remcohaszing