-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
Migrate expect to typescript #7919
Conversation
7e85c0f
to
ad85338
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Overall this looks pretty good, there's just a bit too much any
🙂 Seems like typing MatcherState
correctly would resolve quite a bit of these issue
@@ -146,14 +145,15 @@ class ArrayContaining extends AsymmetricMatcher { | |||
|
|||
class ObjectContaining extends AsymmetricMatcher { | |||
sample: Object; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type this as {[key: string]: unknown}
(or maybe any
), and you shouldn't have to do the type casting later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Great job, this was a tough one.
We can hopefully get rid of some of the any
s at a future time, but since we know the code works it's no big deal
Codecov Report
@@ Coverage Diff @@
## master #7919 +/- ##
==========================================
- Coverage 65.33% 64.89% -0.44%
==========================================
Files 255 255
Lines 9941 10013 +72
Branches 1041 1361 +320
==========================================
+ Hits 6495 6498 +3
- Misses 3195 3199 +4
- Partials 251 316 +65
Continue to review full report at Codecov.
|
Thanks @SimenB! I'm ready to pick up another one once this is good to go. Does this PR require a changelog update? |
Fantastic 😀
It should! Pushed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! There are still quite a bunch of any
and casting we need to take care of later, but for now this is good enough :) I've added some small adjustments in the last commit
@@ -5,7 +5,7 @@ | |||
* LICENSE file in the root directory of this source tree. | |||
*/ | |||
|
|||
// TODO: Move this to `expect` when it's migrated | |||
// TODO: Remove this when whole project is migrated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cannot do it currently, because of export =
stuff, and importing from expect/src/types
seemed fishy (and caused TS errors, oh well who's got time for that XD)
Hey one question before this gets merged in. I was wondering about using the type I didnt use Capital O object because I was referring to this https://www.typescriptlang.org/docs/handbook/declaration-files/do-s-and-don-ts.html It states this
Am I doing this wrongly? Edit: spelling |
IMO we shouldn't use |
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
This PR migrates
expect
to typescript as part of #7807A few points to note.
ts-migration.diff
. You'll have to use the local git tool to view the diff since even github wont display it.Built diff: