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

Implement optional io.Reader in AudioRequest (#303) (#265) #331

Merged
merged 5 commits into from
Jun 5, 2023

Conversation

mdarc
Copy link
Contributor

@mdarc mdarc commented May 28, 2023

This change adds an optional io.Reader to the AudioRequest structure. This addition makes this structure more versatile and allows you to use a different source other than an existing file from your filesystem. It also maintaining backwards compatibility.

I'd be glad to implement the same logic for the other Request types (FileRequest, ImageEditRequest, ImageVariRequest) in preferably subsequent PR's.

@codecov
Copy link

codecov bot commented May 28, 2023

Codecov Report

Merging #331 (4b25634) into master (62eb4be) will increase coverage by 0.68%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #331      +/-   ##
==========================================
+ Coverage   92.36%   93.05%   +0.68%     
==========================================
  Files          19       18       -1     
  Lines         655      648       -7     
==========================================
- Hits          605      603       -2     
+ Misses         37       33       -4     
+ Partials       13       12       -1     
Impacted Files Coverage Δ
audio.go 88.31% <100.00%> (+6.77%) ⬆️

... and 2 files with indirect coverage changes

@0xNF
Copy link

0xNF commented May 29, 2023

It'd be nice to include this feature in the Files Upload, Image Edit, and Image Variation endpoints as well

@sashabaranov
Copy link
Owner

sashabaranov commented May 31, 2023

@mdarc thank you for the PR! Would love to merge it once the test coverage issue is fixed

@mdarc
Copy link
Contributor Author

mdarc commented Jun 1, 2023

@mdarc thank you for the PR! Would love to merge it once the test coverage issue is fixed

Coverage issue fixed. (and np! I'm glad to contribute)

@mdarc
Copy link
Contributor Author

mdarc commented Jun 4, 2023

@mdarc thank you for the PR! Would love to merge it once the test coverage issue is fixed

All checks pass now.

One thing to note is this PR will conflict with #305 although I consider mine to be a more versatile implementation by using a Reader.

Copy link
Owner

@sashabaranov sashabaranov left a comment

Choose a reason for hiding this comment

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

Thank you so much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants