Skip to content

Conversation

@obelisk
Copy link
Owner

@obelisk obelisk commented May 2, 2025

I revisited the Slack API because I wanted to add the ability to fetch user info (specifically timezone data) and I was unhappy with how I previously structured the code. There was no clear separation of responsibilities and it was hard to see how APIs were implemented and especially difficult to add new ones.

This changes all of that and couples the STL and the Plaid library together to throw compile time errors for inconsistencies rather than runtime ApiError::MissingParameter errors.

@obelisk obelisk requested a review from Copilot May 2, 2025 02:40
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR refactors the Slack API integration to improve clarity and enforce compile‑time consistency for API calls by restructuring request builders and error handling.

  • Introduces new helper functions in the Slack API module to build GET and POST requests and centralizes token management.
  • Converts loosely structured parameter mappings into strongly typed JSON structures in the Plaid STL module.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
runtime/plaid/src/apis/slack/api.rs Refactored API calls with dedicated builder functions and improved error handling.
runtime/plaid-stl/src/slack/mod.rs Switched from generic HashMap to defined structs for message and view parameters.
Comments suppressed due to low confidence (1)

runtime/plaid/src/apis/slack/api.rs:45

  • [nitpick] The current implementation of 'uri_params' requires callers to format the entire query string manually. Consider modifying this function to accept key-value pairs for query parameters to make it more robust and less error‑prone.
fn request_builder<T: AsRef<str>>(client: &Client, api: &Apis, token: String, uri_params: Option<T>) -> RequestBuilder {

@obelisk obelisk marked this pull request as ready for review May 2, 2025 03:53
Copy link
Collaborator

@timweri timweri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean!

@obelisk obelisk self-assigned this May 2, 2025
@obelisk obelisk added the API Relating to the API subsystems label May 2, 2025
@obelisk obelisk merged commit 68fe791 into shadow May 3, 2025
9 checks passed
michelemin pushed a commit that referenced this pull request May 28, 2025
* Restructure

* Remove Display impl because it wasn't valuable

* moving more code around; attempting to make it cleaner

* More clean up and variadic type refactor

* Finally made this a red diff

* last bit of reorganization

* Fix comments

* Last little bit of clean up...I hope

* Make Bearer token interpolation happen in a more sensible place

* Add test for Slack webhook

* Add tests for the code I refactored

* Add new get_presence and user_info APIs
michelemin pushed a commit that referenced this pull request May 30, 2025
* Restructure

* Remove Display impl because it wasn't valuable

* moving more code around; attempting to make it cleaner

* More clean up and variadic type refactor

* Finally made this a red diff

* last bit of reorganization

* Fix comments

* Last little bit of clean up...I hope

* Make Bearer token interpolation happen in a more sensible place

* Add test for Slack webhook

* Add tests for the code I refactored

* Add new get_presence and user_info APIs
michelemin pushed a commit that referenced this pull request Jun 5, 2025
* Restructure

* Remove Display impl because it wasn't valuable

* moving more code around; attempting to make it cleaner

* More clean up and variadic type refactor

* Finally made this a red diff

* last bit of reorganization

* Fix comments

* Last little bit of clean up...I hope

* Make Bearer token interpolation happen in a more sensible place

* Add test for Slack webhook

* Add tests for the code I refactored

* Add new get_presence and user_info APIs
harry-anderson pushed a commit to harry-anderson/plaid that referenced this pull request Jun 21, 2025
* Restructure

* Remove Display impl because it wasn't valuable

* moving more code around; attempting to make it cleaner

* More clean up and variadic type refactor

* Finally made this a red diff

* last bit of reorganization

* Fix comments

* Last little bit of clean up...I hope

* Make Bearer token interpolation happen in a more sensible place

* Add test for Slack webhook

* Add tests for the code I refactored

* Add new get_presence and user_info APIs
michelemin pushed a commit that referenced this pull request Jul 9, 2025
* Restructure

* Remove Display impl because it wasn't valuable

* moving more code around; attempting to make it cleaner

* More clean up and variadic type refactor

* Finally made this a red diff

* last bit of reorganization

* Fix comments

* Last little bit of clean up...I hope

* Make Bearer token interpolation happen in a more sensible place

* Add test for Slack webhook

* Add tests for the code I refactored

* Add new get_presence and user_info APIs
obelisk added a commit that referenced this pull request Jul 10, 2025
* Slack API Refactor (#115)

* Restructure

* Remove Display impl because it wasn't valuable

* moving more code around; attempting to make it cleaner

* More clean up and variadic type refactor

* Finally made this a red diff

* last bit of reorganization

* Fix comments

* Last little bit of clean up...I hope

* Make Bearer token interpolation happen in a more sensible place

* Add test for Slack webhook

* Add tests for the code I refactored

* Add new get_presence and user_info APIs

* Add new API for requesting reviewers (#117)

* Split config (#118)

* Split Plaid config across multiple TOML files

* Fix integration tests script

* Fix dockerfiles and sort files alphabetically

* Rebase on top of Slack API changes

* Fix integration tests

* Skip Slack tests if secrets are not available

---------

Co-authored-by: Michele Minelli <michele.minelli@smartcontract.com>

* Move to Wasmer 6 (#126)

* Fix modules gitignore

* Dedicated threads per log type (#121)

* cargo fmt (#122)

* Per-MNR timeout & certificate support (#129)

* Add overwrite functionality to secrets manager binary (#136)

* Support request bodies and headers on `GET` routes (#131)

* Have GET routes accept a body

* Remove content length check

* Allow GET routes to receive headers

* Add an example of building a GET service (#138)

* Keep logbacks and DB in sync (#130)

* Remove unused InternalConfig

* Remove comments that were obsolete after DB migration

* Fix Ord for DelayedMessage

* Ensure consistency between DB and logback heap; add in-memory DB

* Add is_persistent

* Remove shortcut for 0-delay logbacks

* Allow for specifying what a Plaid instance runs (#135)

* Add perPage param when listing npm tokens (#166)

* Add first load test rule (#139)

* Allow for specifying what a Plaid instance runs

* Add first load test rule

* Restart execution threads if they fail (#164)

* Restart execution threads if they fail

* fix

* fix for real

* Bring back shortcut for 0-delay logbacks (#167)

* Update api.rs

* Add Slack API to get a user's DND info (#168)

---------

Co-authored-by: Mitchell Grenier <mitchell@confurious.io>
Co-authored-by: wbssbw <140412462+wbssbw@users.noreply.github.com>
obelisk added a commit that referenced this pull request Jul 10, 2025
* Restructure

* Remove Display impl because it wasn't valuable

* moving more code around; attempting to make it cleaner

* More clean up and variadic type refactor

* Finally made this a red diff

* last bit of reorganization

* Fix comments

* Last little bit of clean up...I hope

* Make Bearer token interpolation happen in a more sensible place

* Add test for Slack webhook

* Add tests for the code I refactored

* Add new get_presence and user_info APIs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

API Relating to the API subsystems

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants