Skip to content

Add types #4

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

Merged
merged 1 commit into from
Jan 6, 2019
Merged

Add types #4

merged 1 commit into from
Jan 6, 2019

Conversation

Rokt33r
Copy link
Contributor

@Rokt33r Rokt33r commented Jan 4, 2019

I don't think the return type should be union type of string and null because typescript assure the argument should be Node, Position or Point.

Otherwise, we need to check the return value every time if it is string or not. That would be horrible.

Or maybe it is nice time to change the behavior of the function so we can sure it always returns a string. (Like returning empty string or throwing an error when it gets an invalid input)

@wooorm
Copy link
Member

wooorm commented Jan 4, 2019

Or maybe it is nice time to change the behavior of the function so we can sure it always returns a string. (Like returning empty string or throwing an error when it gets an invalid input)

Throwing feels a bit harsh to me, but the empty string is 👍

@Rokt33r
Copy link
Contributor Author

Rokt33r commented Jan 4, 2019

Okay, I made a pr for it. #5

@wooorm wooorm added 🧑 semver/major This is a change has pr labels Jan 4, 2019
wooorm pushed a commit that referenced this pull request Jan 5, 2019
Related to GH-4.

Closes GH-5.

Reviewed-by: Christian Murphy <[email protected]>
Reviewed-by: Titus Wormer <[email protected]>
@wooorm wooorm removed the has pr label Jan 6, 2019
@wooorm wooorm merged commit 8e73aff into syntax-tree:master Jan 6, 2019
@Rokt33r Rokt33r deleted the types branch January 7, 2019 01:24
@wooorm wooorm added ☂️ area/types This affects typings ⛵️ status/released 🦋 type/enhancement This is great to have labels Aug 11, 2019
@wooorm wooorm added the 💪 phase/solved Post is done label Apr 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
☂️ area/types This affects typings 💪 phase/solved Post is done 🧑 semver/major This is a change 🦋 type/enhancement This is great to have
Development

Successfully merging this pull request may close these issues.

3 participants