Migrate expect to typescript#7919
Conversation
7e85c0f to
ad85338
Compare
SimenB
left a comment
There was a problem hiding this comment.
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.
type this as {[key: string]: unknown} (or maybe any), and you shouldn't have to do the type casting later
natealcedo
left a comment
There was a problem hiding this comment.
Just a couple more changes
| const {pass} = matcher( | ||
| other, | ||
| ...(this.sample as [unknown, unknown]), | ||
| ) as SyncExpectationResult; |
There was a problem hiding this comment.
Just copying over from the original code. This typing seems more accurate than my previous change. comments are appreciated
SimenB
left a comment
There was a problem hiding this comment.
Thanks! Great job, this was a tough one.
We can hopefully get rid of some of the anys 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 |
thymikee
left a comment
There was a problem hiding this comment.
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
| */ | ||
|
|
||
| // TODO: Move this to `expect` when it's migrated | ||
| // TODO: Remove this when whole project is migrated |
There was a problem hiding this comment.
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
expectto 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:
Details