Typescript migration #113
Typescript migration #113
Conversation
|
Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward? |
|
Not stale
…________________________________
From: stale[bot] <[email protected]>
Sent: Monday, July 1, 2019 6:21:21 PM
To: probot/smee-client
Cc: wolfy1339; Author
Subject: Re: [probot/smee-client] Typescript migration (#113)
Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub<#113?email_source=notifications&email_token=ABDB6FMQN5K6KQPQLLSTBOTP5J7ODA5CNFSM4HC4VBFKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODY7QLEY#issuecomment-507446675>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ABDB6FNTIKES5IY2EUXTXZTP5J7ODANCNFSM4HC4VBFA>.
|
|
Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward? |
|
Not stale |
|
Ping @JasonEtco |
|
Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward? |
|
Not stale |
|
Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward? |
|
Still relevant
…On 2020-03-28 2:48 a.m., stale[bot] wrote:
Is this still relevant? If so, what is blocking it? Is there anything
you can do to help move it forward?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#113 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABDB6FOHQQCP4VVKZ6T37ETRJWMU3ANCNFSM4HC4VBFA>.
|
|
Ping @JasonEtco @tcbyrd |
|
Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward? |
|
Still relevant
|
Change Imports Define class properties Fix all remaining errors
Since `async`/`await` was introduced in ES2017, why not take advantage of static class members
index.ts
Outdated
| constructor ({ source, target, logger = console }) { | ||
| source: string; | ||
| target: string; | ||
| logger: Console; |
There was a problem hiding this comment.
In probot we are more precise about this type:
Shall we do the same here?
There was a problem hiding this comment.
Yes, indeed. they should be updated to those ones
| @@ -24,7 +35,7 @@ class Client { | |||
|
|
|||
| delete data.query | |||
|
|
|||
| const req = superagent.post(target).send(data.body) | |||
| const req = superagent.post(url.format(target)).send(data.body) | |||
There was a problem hiding this comment.
can you recall why you did this change?
There was a problem hiding this comment.
I don't remember exactly.
But removing it causes errors
Argument of type 'UrlWithParsedQuery' is not assignable to parameter of type 'string'.
| @@ -36,7 +47,7 @@ class Client { | |||
| if (err) { | |||
| this.logger.error(err) | |||
| } else { | |||
| this.logger.info(`${req.method} ${req.url} - ${res.statusCode}`) | |||
| this.logger.info(`${req.method} ${req.url} - ${res.status}`) | |||
There was a problem hiding this comment.
can you recall why you did this change?
There was a problem hiding this comment.
Same comment as above.
Property 'statusCode' does not exist on type 'Response'.
|
As we don't have good test coverage yet (see #155), did you try it locally if this actually works? 😁 |
There was a problem hiding this comment.
I tested it locally and it all looks good to me. Thanks for your patience with us @wolfy1339 💐
Fixes #74