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(gen): no NPE on default stream responses #875

Merged
merged 2 commits into from
Apr 26, 2023

Conversation

natehart
Copy link
Contributor

@natehart natehart commented Apr 24, 2023

The current implementation of the templated stream response Read(byte[]) function uses the Data io.Reader field without first checking for nil.

Users who want to return a value with an empty body are forced to explicitly specify an empty io.Reader rather than simply ignore the field and get use from the empty struct.

This commit adds a nil check before using the Data field and, if it is nil, responds as if it were an empty io.Reader instead of panicking.

This will hopefully make this library slightly more ergonomic to use.

Copy link
Member

@tdakkota tdakkota left a comment

Choose a reason for hiding this comment

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

Push chore: commit generated files commit to this PR branch, please.

The change itself looks good to me. Thank you for contribution.

@codecov
Copy link

codecov bot commented Apr 24, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (c1ded8d) 75.75% compared to head (301b6db) 75.75%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #875   +/-   ##
=======================================
  Coverage   75.75%   75.75%           
=======================================
  Files         189      189           
  Lines       13854    13854           
=======================================
  Hits        10495    10495           
  Misses       2837     2837           
  Partials      522      522           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@ernado
Copy link
Member

ernado commented Apr 25, 2023

Hi! Please

  1. Change commit message with lowercase No, commitlint requries it
  2. Add commit from feat(gen): no NPE on default stream responses #876
  3. Add your commit email to your github account or change it, so it is "verified" :)

@ernado ernado enabled auto-merge April 25, 2023 06:46
@ernado ernado changed the title feat(gen): No NPE on default stream responses feat(gen): no NPE on default stream responses Apr 25, 2023
auto-merge was automatically disabled April 25, 2023 16:54

Head branch was pushed to by a user without write access

@natehart natehart requested a review from tdakkota April 25, 2023 17:01
@ernado
Copy link
Member

ernado commented Apr 25, 2023

I think you need to rebase instead of merge, so merge commit won’t be in your branch 🙃

@natehart
Copy link
Contributor Author

Derp. That's what I get for using GitHub's options...

@natehart natehart force-pushed the stream-template branch 2 times, most recently from 20ec958 to 56268c1 Compare April 25, 2023 21:15
The current implementation of the templated stream response `Read(byte[])`
function uses the `Data io.Reader` field without first checking for `nil`.

Users who want to return a value with an empty body are forced to explicitly
specify an empty `io.Reader` rather than simply ignore the field and get use
from the empty struct.

This commit adds a `nil` check before using the `Data` field and, if it is
`nil`, responds as if it were an empty `io.Reader` instead of panicking.

This will hopefully make this library slightly more ergonomic to use.
@tdakkota tdakkota enabled auto-merge April 26, 2023 01:34
@tdakkota tdakkota merged commit 38e6569 into ogen-go:main Apr 26, 2023
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