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

Use Slack's library for calling HTTP endpoints #185

Merged
merged 10 commits into from Aug 29, 2019

Conversation

@Half-Shot
Copy link
Contributor

commented Aug 28, 2019

Sorry about the size of this one, but it was sort of unavoidable.

This PR changes the bridge significantly so that we now rely on @slack/web-api rather than doing HTTP calls ourselves, which reduces the amount of maintenance needed somewhat. API responses now also are typed and based off the slack documentation.

This PR can be treated separate from #164, but should be merged in beforehand ideally.

image

  • pictured: reviewer wondering why these PRs are so massive

@Half-Shot Half-Shot requested a review from matrix-org/bridges Aug 28, 2019

@Half-Shot

This comment has been minimized.

Copy link
Contributor Author

commented Aug 28, 2019

(oh it broke, one second)

@Half-Shot Half-Shot requested review from matrix-org/bridges and removed request for matrix-org/bridges Aug 28, 2019

@anoadragon453

This comment has been minimized.

Copy link
Member

commented Aug 28, 2019

image

oh god why

+455 −1,521

Half-Shot added 2 commits Aug 28, 2019
Apply fixes from review
s/webClient/slackClient/
remove superflous statements

@Half-Shot Half-Shot force-pushed the hs/slack-http branch from fbe62dc to b4c6d6b Aug 28, 2019

@turt2live
Copy link
Member

left a comment

it's still far too difficult to review this, but there's nothing glaringly obvious at being wrong. With all these changes the bridge is going to need significant testing before release.

@Half-Shot

This comment has been minimized.

Copy link
Contributor Author

commented Aug 29, 2019

With all these changes the bridge is going to need significant testing before release.

This is 100% the plan.

@Half-Shot Half-Shot merged commit b4c6d6b into develop Aug 29, 2019

2 checks passed

buildkite/matrix-appservice-slack Build #60 passed (49 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
4 participants
You can’t perform that action at this time.