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

feat: Use type-guards from typedescriptor. #351

Merged
merged 1 commit into from
May 17, 2021
Merged

Conversation

yeldiRium
Copy link
Contributor

No description provided.

Copy link
Member

@goloroden goloroden left a comment

Choose a reason for hiding this comment

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

@yeldiRium Looks pretty good, thanks a lot 😊

Just one thought – Type | 'result' is used pretty often. Would it make sense to wrap this into a type of its own?

However, I haven't marked this as Changes requested, so if you are fine with the way it is, feel free to merge, otherwise just update 😉

@yeldiRium
Copy link
Contributor Author

@yeldiRium Looks pretty good, thanks a lot

Just one thought – Type | 'result' is used pretty often. Would it make sense to wrap this into a type of its own?

This might make sense. But the type Type | 'result' is only used three times in actual library code (excluding tests) and I would like to keep the imports as they are, because currently it is obvious that the type Type comes from the typedescriptor package and is then extended. If we wrap this in a library-owned type and import that, the connection to typedescriptor is one step removed.

Do you think for three occurences we can keep it this way?

@goloroden goloroden merged commit 6cb20a5 into main May 17, 2021
@goloroden goloroden deleted the feat/extract-typeguards branch May 17, 2021 07:53
@goloroden
Copy link
Member

Yep 😉

goloroden pushed a commit that referenced this pull request May 17, 2021
# [6.1.0](6.0.1...6.1.0) (2021-05-17)

### Features

* Use type-guards from package typedescriptor. ([#351](#351)) ([6cb20a5](6cb20a5))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants