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

Improving typing of external row API #12100

Merged
merged 6 commits into from
Oct 18, 2023

Conversation

mike12345567
Copy link
Collaborator

@mike12345567 mike12345567 commented Oct 18, 2023

Description

Improving the typing around the ExternalRequest object, which has implications throughout the row API and SDK, cleaning up where possible based on it.

The main change here is making sure that Operation.READ has a specific response (as it already did) and all other operations have their own specific response (as they already did). The types just did not allow for this knowledge to be fully transferred through the layers, requiring some gymnastics when using the class.

Feature branch env

Feature Branch Link

…lications throughout the row API and SDK, cleaning up where possible based on it.
@mike12345567 mike12345567 self-assigned this Oct 18, 2023
Copy link
Collaborator

@samwho samwho left a comment

Choose a reason for hiding this comment

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

Much clearer, thanks for sorting this Michael! 🙏

@codecov-commenter
Copy link

codecov-commenter commented Oct 18, 2023

Codecov Report

Merging #12100 (5ff6b7a) into master (f9bff08) will increase coverage by 6.86%.
Report is 4 commits behind head on master.
The diff coverage is 100.00%.

❗ Current head 5ff6b7a differs from pull request most recent head a60690f. Consider uploading reports for the commit a60690f to get more accurate results

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff             @@
##           master   #12100      +/-   ##
==========================================
+ Coverage   68.50%   75.37%   +6.86%     
==========================================
  Files         585      326     -259     
  Lines       21839    13904    -7935     
  Branches     4409     2931    -1478     
==========================================
- Hits        14961    10480    -4481     
+ Misses       6364     3198    -3166     
+ Partials      514      226     -288     
Files Coverage Δ
.../server/src/api/controllers/row/ExternalRequest.ts 92.87% <100.00%> (+0.01%) ⬆️
...ackages/server/src/api/controllers/row/external.ts 93.75% <100.00%> (ø)
packages/server/src/sdk/app/rows/external.ts 80.00% <100.00%> (ø)
packages/server/src/sdk/app/rows/search.ts 100.00% <100.00%> (ø)
...ackages/server/src/sdk/app/rows/search/external.ts 86.45% <100.00%> (ø)
...ackages/server/src/sdk/app/rows/search/internal.ts 85.24% <100.00%> (-0.12%) ⬇️

... and 260 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@mike12345567 mike12345567 added the firestorm Data/Infra/Revenue Team label Oct 18, 2023
@mike12345567 mike12345567 merged commit dde1cb7 into master Oct 18, 2023
9 of 10 checks passed
@mike12345567 mike12345567 deleted the fix/improve-external-request-typing branch October 18, 2023 12:03
@github-actions github-actions bot locked and limited conversation to collaborators Oct 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
firestorm Data/Infra/Revenue Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants