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(expect-type): add guards & asserts getters #231

Merged
merged 5 commits into from
Jun 29, 2021
Merged

feat(expect-type): add guards & asserts getters #231

merged 5 commits into from
Jun 29, 2021

Conversation

GerkinDev
Copy link
Contributor

Test the type asserted or guarded by the subject function.

@codecov-io
Copy link

codecov-io commented Jan 15, 2021

Codecov Report

Merging #231 (d14e705) into main (72630da) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##              main      #231   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           41        41           
  Lines          721       721           
  Branches       121       121           
=========================================
  Hits           721       721           
Impacted Files Coverage Δ
packages/expect-type/src/index.ts 100.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 72630da...d14e705. Read the comment docs.

Copy link
Owner

@mmkal mmkal left a comment

Choose a reason for hiding this comment

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

Hi @GerkinDev, thanks for the PR!

I like this and would be happy to accept. I'm not so sure about lumping is T together with asserts is T though. What about two helpers:

expectTypeOf(myFunc).guard.toBeString()
expectTypeOf(myOtherFunc).assertion.toBeString()

That way we wouldn't need the GuardedType either. What do you think?

One other thing - if you run npm run lint -- --fix in the project folder, it'll automatically update the docs to include your new test. Worth giving the test a readme-appropriate name too.

@GerkinDev
Copy link
Contributor Author

Well, in that case, and to stay consistent with your already existing getters, I'd name those functions guards & asserts, since you already have returns and resolves.

@GerkinDev
Copy link
Contributor Author

GerkinDev commented Jan 15, 2021

Another thing: when running npm run lint -- --fix (or even just npm run lint), I have an error about rig not being found. Not really sure what I'm supposed to do. Am I supposed to link globally the content of tools/rig ? It would worth a small note in the install README (assuming you're planning to stay OS/PR welcoming).

@mmkal
Copy link
Owner

mmkal commented Jan 15, 2021

Hmm yes I suppose you're right re guards vs guardconsistency. The aim is to have the assertions read like a sentence though. And "expect type of foo guards to be string" makes a bit less sense than "expect type of foo guard to be string". I think there's similar inconsistency with .exclude and .extract.

While I have a (quick) think about that one, the lint problem: you're absolutely right, this repo is quite cryptic for OSS contributors. To get the various dev dependencies depends on rush. So I think what you'd need to do:

npm install --global @microsoft/rush
rush install # install all repo dependencies
rush build --to expect-type # build the repo
rush lint --to expect-type --fix

The first time you run these it might take a little while.

I'll add to the docs of each package in this monorepo soon.

@GerkinDev GerkinDev changed the title feat(expect-type): add guardedType getter feat(expect-type): add guards & asserts getters Jan 15, 2021
}
expectTypeOf(assertNumber).asserts.toBeNumber()

const isError = (v: any): v is string => typeof v === 'string'
Copy link
Owner

Choose a reason for hiding this comment

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

Shouldn't this be named isString?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dammit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 30b3c4e

@GerkinDev
Copy link
Contributor Author

Hey there,

Any news on this ?

@mmkal
Copy link
Owner

mmkal commented Jun 26, 2021

@GerkinDev sorry for the long delay. I'll merge and publish this in the next few days.

@GerkinDev
Copy link
Contributor Author

No problem, I was off myself for a while ;)

@mmkal mmkal merged commit 1a3165a into mmkal:main Jun 29, 2021
@chriskrycho
Copy link

Just wanted to offer a drive-by "thank you!" for this as a user of the library. These are nice additions!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants