search

inveniosoftware / flask-resources

REST APIs for Flask
https://flask-resources.readthedocs.io
MIT License
3 stars 21 forks source link

Design comments #26

Closed lnielsen closed 4 years ago

lnielsen commented 4 years ago
ppanero commented 4 years ago

Answering some in-line. For the ones solved in this PR, first requires rebase on top of https://github.com/inveniosoftware/flask-resources/pull/31 Since the code changed a bit.

  • Removal of patch loader (discussed IRL)

Fixed in https://github.com/inveniosoftware/flask-resources/pull/31 . Removed.

  • Blueprint names: I would only add suffixes to blueprint names that have multiple endpoints (i.e. only collection).

Fixed in this PR. Need to test for collisions. Otherwise I'm okay with it.

  • Naming: Resource -> ResourceView (discussed IRL, to be rediscussed)

Fixed in this PR. If not moved to the views folder, needs a better docstring explaining why 😅.

  • Resource. should take same parameters as the MethodView.get/put/post/patch/...

Fixed in this PR.

  • Accept is only a request header, but not a response header.

Fixed in this PR.

  • Loaders should put data on the resource request context and not pass it as arguments (to be further discussed re. how loaders should look like)

Fixed in https://github.com/inveniosoftware/flask-resources/pull/31.

  • Can MethodView.decorators be used in BaseView?

No, since it is a cls function. However, as it is now, each function is being decorated on each call which is a bit of an overhead.

  • DELETE could potentially take a payload (for tombstone pages)

In https://github.com/inveniosoftware/flask-resources/pull/31 the loaders allow data to be passed optionally (default to required though). So both DELETE sending a tombstone page data or DELETE with no body could be implemented.

  • Content negotiation can it be applied to all methods (e.g. DELETE might not take)

Would only happen if no response body is sent. However, as it is now, content negotiation would fail if no default.

ppanero commented 4 years ago

The changes have been already merged as part of different PRs