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

Multipart #38

Merged
merged 13 commits into from Dec 11, 2017

Conversation

2 participants
@yohanboniface
Member

yohanboniface commented Nov 30, 2017

No description provided.

yohanboniface and others added some commits Nov 30, 2017

@davidbgk davidbgk referenced this pull request Dec 8, 2017

Closed

WIP: Parse multipart forms #14

@@ -95,6 +101,29 @@ response.json = {'some': 'dict'}
response.json = [{'some': 'dict'}, {'another': 'one'}]
```
### Multipart
Responsible of the parsing of `request.body`.

This comment has been minimized.

@yohanboniface

yohanboniface Dec 8, 2017

Member

"... of multipart encoded request.body" ?

#### Methods
- **parse(content_type: str)**: returns a tuple

This comment has been minimized.

@yohanboniface

yohanboniface Dec 8, 2017

Member

Not sure the name is that good: it should imho reflects the two steps workflow, i.e. "init" then "feed_data" as many times as needed. So maybe init.
Or we use the __init__ (which does not return anything then), and we retrieve form and files self._form = parser.form.

### Form
Allow to access casted POST parameters from `request.body`.

This comment has been minimized.

@yohanboniface

yohanboniface Dec 8, 2017

Member

We should clarify that Form inherits from Query, with all the goodies.

### Files
Allow to access POSTed files from `request.body`.

This comment has been minimized.

@yohanboniface

yohanboniface Dec 8, 2017

Member

We should document get and list method (from Multidict).

This comment has been minimized.

@davidbgk

davidbgk Dec 8, 2017

Contributor

Done.

def feed_data(self, data: bytes):
self._parser.feed_data(data)
def on_body_begin(self):

This comment has been minimized.

@yohanboniface

yohanboniface Dec 8, 2017

Member

We may make all those event listener optional, multifruits side.

try:
return json.loads(self.body.decode())
except (UnicodeDecodeError, JSONDecodeError):
return {}

This comment has been minimized.

@yohanboniface

yohanboniface Dec 8, 2017

Member

Maybe None would be a better default?

This comment has been minimized.

@davidbgk

davidbgk Dec 8, 2017

Contributor

It would raise an error in case of request.json.get('foo').

@@ -25,6 +79,30 @@ class Client:
def __init__(self, app):
self.app = app
def handle_files(self, kwargs):

This comment has been minimized.

@yohanboniface

yohanboniface Dec 8, 2017

Member

Not related to this PR, but let's have a dedicated doc page (how-to) for "testing".

This comment has been minimized.

@davidbgk

davidbgk Dec 8, 2017

Contributor

Done!

@pyrates pyrates deleted a comment from yohanboniface Dec 8, 2017

davidbgk added some commits Dec 8, 2017

@davidbgk davidbgk changed the title from [WIP] multipart to Multipart Dec 8, 2017

@property
def files(self):
if self._files is None:
self._parse_multipart()

This comment has been minimized.

@yohanboniface

yohanboniface Dec 11, 2017

Member

We should handle the case where it's not a multipart body.

@@ -50,7 +50,7 @@
async def test_post_json(client, app):
@app.route('/test', methods=['POST'])
async def get(req, resp):
async def post(req, resp):

This comment has been minimized.

@yohanboniface

yohanboniface Dec 11, 2017

Member

Weird that assert resp.status == HTTPStatus.OK hasn't raise before :s

davidbgk added some commits Dec 11, 2017

@@ -443,9 +475,8 @@ def protocol(app):
@app.route('/test', methods=['POST'])
async def post(req, resp):
assert req.json == {}
resp.body = b'done'
assert req.json

This comment has been minimized.

@yohanboniface

yohanboniface Dec 11, 2017

Member

raise to make it clearer that we should not hit that piece of code?

This comment has been minimized.

@davidbgk

davidbgk Dec 11, 2017

Contributor

Actually, that's the instantiation that raises the error, maybe better to do resp.body = req.json?

@davidbgk davidbgk merged commit 55da090 into master Dec 11, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@yohanboniface yohanboniface deleted the multipart branch Dec 11, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment