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

feat: add ok boolean to DWNResponseStatus #959

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

Toheeb-Ojuolape
Copy link

What type of PR is this? (check all applicable)

  • ♻️ Refactor
  • ✨ New Feature
  • 🐛 Bug Fix
  • 📝 Documentation Update
  • 👷 Example Application
  • 🧑‍💻 Code Snippet
  • 🎨 Design
  • 📖 Content
  • 🧪 Tests
  • 🔖 Release
  • 🚩 Other

Description

This PR adds an ok boolean to the DWNResponseStatus for requests that were successful

Related Tickets & Documents

Resolves #904

Mobile & Desktop Screenshots/Recordings

Added code snippets?

  • 👍 yes
  • 🙅 no, because they aren't needed

Added tests?

  • 👍 yes
  • 🙅 no, because they aren't needed
  • 🙋 no, because I need help

[optional] Are there any post-deployment tasks we need to perform?

Please run npm install or make sure @web5/crypto package is installed before testing

[optional] What gif best describes this PR or how it makes you feel?

Demo GIF

Copy link

changeset-bot bot commented Oct 21, 2024

⚠️ No Changeset found

Latest commit: 35931df

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@taniashiba taniashiba added the hacktoberfest For the hacking month of October label Oct 23, 2024
@LiranCohen
Copy link
Member

Hey @Toheeb-Ojuolape! Thanks for this! Great work, if you could add some tests that confirm the behavior that would be great!

packages/api/src/dwn-api.ts Outdated Show resolved Hide resolved
@Toheeb-Ojuolape
Copy link
Author

Hey @Toheeb-Ojuolape! Thanks for this! Great work, if you could add some tests that confirm the behavior that would be great!

Hi @LiranCohen I've added some tests. Hope you find it acceptable. Thank you

@LiranCohen
Copy link
Member

Hey @Toheeb-Ojuolape! Thanks for this, I'm seeing a few Type errors in the test runner for some reason, will pull it down in a bit to confirm myself.

For the tests, would you mind creating it's own specific test for each interface that describes the behavior. That way it's easy to find the behavior of the specific test. As well as adding a negative test case (can be within the postiive one as well) to show it returning false.

Something along the lines of `status ok boolean should be true if the response is ok'.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest For the hacking month of October small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement an ok boolean to signal whether an API request was successful.
4 participants