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

Switch to Typescript #152

Merged
merged 86 commits into from Aug 27, 2019

Conversation

@Half-Shot
Copy link
Contributor

commented Jul 4, 2019

This PR largely doesn't change the logic of the bridge already in develop but fully converts everything into Typescript. Where possible, functions are now async.

Changes made:

  • The admin command handler has been totally refactored and is using yargs for parsing.

@Cadair can you also review.

Note to reviewers: I don't expect you to review the whole diff because it's huge and mostly unchanged, however if you could glance through for any obvious mistakes that would be appreciated.

@Half-Shot

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2019

Update: I've made the changes where I can for @turt2live, refactoring in places. I still need to add more comments. I also need to pull in changes from develop.

@Half-Shot Half-Shot requested a review from turt2live Aug 8, 2019

tslint.json Outdated Show resolved Hide resolved
@turt2live
Copy link
Member

left a comment

syntax largely looks okay, however I do want to do a more complete review. So far I've only scrolled and glanced at things without actually reading the code.

@turt2live turt2live self-requested a review Aug 12, 2019

src/AdminCommands.ts Outdated Show resolved Hide resolved
src/AdminCommands.ts Outdated Show resolved Hide resolved

@turt2live turt2live self-requested a review Aug 12, 2019

Half-Shot added 3 commits Aug 12, 2019
Merge in some fixes applied during testing
Squashed commit of the following:

commit ce30945
Author: Half-Shot <will@half-shot.uk>
Date:   Fri Aug 16 12:58:03 2019 +0100

    Add items property for working reactions

commit bba7b98
Author: Half-Shot <will@half-shot.uk>
Date:   Fri Aug 16 12:47:16 2019 +0100

    Add missing reaction statement

commit 0625b89
Author: Half-Shot <will@half-shot.uk>
Date:   Fri Aug 16 12:38:40 2019 +0100

    Remove some else statements / remove !res.ok

    Slack is inconsistent with it's return messages, sometimes returning just "ok"

commit a208c2a
Author: Half-Shot <will@half-shot.uk>
Date:   Fri Aug 16 12:35:41 2019 +0100

    s/StoreEvent/StoredEvent/

commit 5aa0ce6
Author: Half-Shot <will@half-shot.uk>
Date:   Fri Aug 16 11:08:06 2019 +0100

    Ensure that findParentReply does not recurse indefinitely

commit c981710
Author: Half-Shot <will@half-shot.uk>
Date:   Fri Aug 16 11:07:51 2019 +0100

    Make functions private, and await them
@Half-Shot

This comment has been minimized.

Copy link
Contributor Author

commented Aug 22, 2019

@turt2live Can we reach a conclusion on this? I've run the bridge in the wild and it seems to perform identically to the develop variant. I know it's a large PR but it's going to be required for other urgent work going forward.

@turt2live

This comment has been minimized.

Copy link
Member

commented Aug 22, 2019

I really just don't have enough free time to review this. I understand that it might perform the same, but it would be irresponsible of me to not check the code :(

If it's urgent, please get Neil involved to balance the scales against all the other work in flight.

@turt2live
Copy link
Member

left a comment

generally lgtm - please review all comment threads both inside and outside of this review, as some have not been dealt with in prior iterations.

src/AdminCommand.ts Outdated Show resolved Hide resolved
src/AdminCommand.ts Outdated Show resolved Hide resolved
src/AdminCommand.ts Outdated Show resolved Hide resolved
src/Main.ts Outdated Show resolved Hide resolved
src/Main.ts Outdated Show resolved Hide resolved
src/SlackGhost.ts Outdated Show resolved Hide resolved
@turt2live

This comment has been minimized.

Copy link
Member

commented Aug 23, 2019

also I haven't audited the code at all: as I mentioned oob, if you've put something nasty in here then we'll sue :p

@Half-Shot Half-Shot merged commit 9d93f6c into develop Aug 27, 2019

2 checks passed

buildkite/matrix-appservice-slack Build #30 passed (59 seconds)
Details
ci/dockercloud Your tests passed in Docker Cloud
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.