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

Setup Content-Type to application/octet-stream by default #1124

Merged
merged 8 commits into from
Aug 26, 2016
Merged

Setup Content-Type to application/octet-stream by default #1124

merged 8 commits into from
Aug 26, 2016

Conversation

avamsi
Copy link
Contributor

@avamsi avamsi commented Aug 25, 2016

Related issue number

#944

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes

@avamsi
Copy link
Contributor Author

avamsi commented Aug 26, 2016

@asvetlov Just saw the failing tests. Sorry about that.
If you can take a look at the code changes and confirm everything's okay, I'll update affected tests accordingly.

@@ -774,7 +774,7 @@ def __init__(self, *, body=None, status=200,
self.set_tcp_cork(True)

if body is not None and text is not None:
raise ValueError("body and text are not allowed together.")
raise ValueError('body and text are not allowed together')
Copy link
Member

Choose a reason for hiding this comment

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

Please use " for exception messages

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought the difference in quotes usage was due to different contributors. I'll change it back

@asvetlov
Copy link
Member

Please don't forget about:

I think this code should be in StreamResponse ctor.
Content-Type should be application/octet-stream if not explicitly specified.
It's true for all responses.

See https://tools.ietf.org/html/rfc7231#section-3.1.1.5

@codecov-io
Copy link

Current coverage is 98.15% (diff: 100%)

Merging #1124 into master will increase coverage by <.01%

@@             master      #1124   diff @@
==========================================
  Files            28         28          
  Lines          6387       6391     +4   
  Methods           0          0          
  Messages          0          0          
  Branches       1073       1074     +1   
==========================================
+ Hits           6269       6273     +4   
  Misses           63         63          
  Partials         55         55          

Powered by Codecov. Last update 19c0f31...ce207aa

@asvetlov asvetlov merged commit 8e9e73e into aio-libs:master Aug 26, 2016
@asvetlov
Copy link
Member

Thanks!

@asvetlov asvetlov changed the title Fix for #944 Setup Content-Type to application/octet-stream by default Aug 26, 2016
@avamsi avamsi deleted the patch-1 branch August 27, 2016 03:31
@lock
Copy link

lock bot commented Oct 29, 2019

This thread has been automatically locked since there has not been
any recent activity after it was closed. Please open a new issue for
related bugs.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.

@lock lock bot added the outdated label Oct 29, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants