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 parseSuccessResponseBody Option #602

Merged
merged 18 commits into from
Jul 11, 2023

Conversation

Uzlopak
Copy link
Contributor

@Uzlopak Uzlopak commented Jul 7, 2023

This is currently more a poc than the final implementation.

Resolves #601


Behavior

Before the change?

After the change?

Other information


Additional info

Pull request checklist

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Added the appropriate label for the given change

Does this introduce a breaking change?

Please see our docs on breaking changes to help!

  • Yes (Please add the Type: Breaking change label)
  • No

If Yes, what's the impact:

  • N/A

Pull request type

Please add the corresponding label for change this PR introduces:

  • Bugfix: Type: Bug
  • Feature/model/API additions: Type: Feature
  • Updates to docs or samples: Type: Documentation
  • Dependencies/code cleanup: Type: Maintenance

@wolfy1339 wolfy1339 added the Type: Feature New feature or request label Jul 7, 2023
@wolfy1339 wolfy1339 changed the title add asStream Option feat: add asStream Option Jul 7, 2023
@wolfy1339
Copy link
Member

The type updates for the function need to made in @octokit/types

That can be handled later when this is greenlit

@gr2m gr2m marked this pull request as draft July 7, 2023 00:58
src/fetch-wrapper.ts Outdated Show resolved Hide resolved
@Uzlopak Uzlopak changed the title feat: add asStream Option feat: add parseResponse Option Jul 8, 2023
@Uzlopak
Copy link
Contributor Author

Uzlopak commented Jul 8, 2023

Ok i modified it. Now test coverage is not 100% but i would add corresponding unit tests, if you say, this is the way to go.

src/fetch-wrapper.ts Outdated Show resolved Hide resolved
src/fetch-wrapper.ts Outdated Show resolved Hide resolved
@Uzlopak
Copy link
Contributor Author

Uzlopak commented Jul 8, 2023

Tbh i am not that happy about the name parseResponse. Maybe call it parseResponseBody or parseResponseData?

src/fetch-wrapper.ts Outdated Show resolved Hide resolved
Co-authored-by: wolfy1339 <4595477+wolfy1339@users.noreply.github.com>
@gr2m
Copy link
Contributor

gr2m commented Jul 8, 2023

Tbh i am not that happy about the name parseResponse. Maybe call it parseResponseBody or parseResponseData?

Agree, that's better. We could even make it more explicit and call it parseSuccessResponseBody to make clear that the option only applies to success responses, to address our discussion above.

Copy link
Contributor Author

@Uzlopak Uzlopak left a comment

Choose a reason for hiding this comment

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

blub

src/fetch-wrapper.ts Outdated Show resolved Hide resolved
src/fetch-wrapper.ts Outdated Show resolved Hide resolved
src/fetch-wrapper.ts Outdated Show resolved Hide resolved
src/fetch-wrapper.ts Outdated Show resolved Hide resolved
src/fetch-wrapper.ts Outdated Show resolved Hide resolved
src/fetch-wrapper.ts Outdated Show resolved Hide resolved
src/fetch-wrapper.ts Outdated Show resolved Hide resolved
src/fetch-wrapper.ts Outdated Show resolved Hide resolved
test/request.test.ts Outdated Show resolved Hide resolved
@Uzlopak Uzlopak changed the title feat: add parseResponse Option feat: add parseResponseBody Option Jul 8, 2023
@Uzlopak Uzlopak changed the title feat: add parseResponseBody Option feat: add parseResponse Option Jul 8, 2023
@Uzlopak Uzlopak changed the title feat: add parseResponse Option feat: add parseSuccessResponseBody Option Jul 8, 2023
README.md Outdated Show resolved Hide resolved
src/fetch-wrapper.ts Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Co-authored-by: Gregor Martynus <39992+gr2m@users.noreply.github.com>
@Uzlopak
Copy link
Contributor Author

Uzlopak commented Jul 9, 2023

In the whole code base it is response.data.

@wolfy1339
Copy link
Member

In the whole code base it is response.data.

That is handled by @octokit/endpoint, it's a different parameter than body.

https://github.com/octokit/request.js/pull/599/files#r1256484656

@Uzlopak Uzlopak marked this pull request as ready for review July 9, 2023 14:35
Copy link
Contributor

@gr2m gr2m left a comment

Choose a reason for hiding this comment

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

Looks good to me now!

@Uzlopak Can you confirm the change works for your usecase? To test you can run npm run build and then import { request } from the pkg/ folder

@wolfy1339
Copy link
Member

I pushed the update to @octokit/types, simply need to remove the manual definition here and upgrade @octokit/types

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Jul 11, 2023

@wolfy1339
done

@wolfy1339 wolfy1339 merged commit 110f3c4 into octokit:main Jul 11, 2023
@github-actions
Copy link

🎉 This PR is included in version 8.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@gr2m
Copy link
Contributor

gr2m commented Jul 11, 2023

@Uzlopak thank you for seeing this through. I'm sorry I closed #601 prematurely, that was wrong the wrong call. I'm glad you didn't give up on it, the resulting pull request turned out really great and it unlocked a use case I thought we couldn't really cover without adding a lot of complexity to the core functionality of Octokit. Thank you 💐

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Jul 13, 2023

@gr2m

Thank you for your kind words. :)

@Uzlopak Uzlopak deleted the asStream-option branch November 9, 2023 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released Type: Feature New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[FEAT]: Allow streaming of the data
3 participants