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

Design comments #26

Closed
wants to merge 1 commit into from
Closed

Conversation

lnielsen
Copy link
Member

@lnielsen lnielsen commented Jun 17, 2020

  • Removal of patch loader (discussed IRL)
  • Blueprint names: I would only add suffixes to blueprint names that have multiple endpoints (i.e. only collection).
  • Naming: Resource -> ResourceView (discussed IRL, to be rediscussed) (Closes resources: rename to ResourceView #18)
  • Resource. should take same parameters as the MethodView.get/put/post/patch/...
  • Accept is only a request header, but not a response header.
  • 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)
  • Can MethodView.decorators be used in BaseView?
  • DELETE could potentially take a payload (for tombstone pages)
  • Content negotiation can it be applied to all methods (e.g. DELETE might not take)

@lnielsen lnielsen added this to In progress in InvenioRDM June Board via automation Jun 17, 2020
@lnielsen lnielsen marked this pull request as draft June 17, 2020 16:28
@ppanero
Copy link
Member

ppanero commented Jun 18, 2020

Answering some in-line. For the ones solved in this PR, first requires rebase on top of #31 Since the code changed a bit.

  • Removal of patch loader (discussed IRL)

Fixed in #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 #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 #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.

This was referenced Jun 18, 2020
@ppanero ppanero moved this from In progress to Done in InvenioRDM June Board Jun 19, 2020
@ppanero
Copy link
Member

ppanero commented Jun 24, 2020

The changes have been already merged as part of different PRs

@ppanero ppanero closed this Jun 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

resources: rename to ResourceView
2 participants