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: Evaluate x-viur-bonelist on default viewSkel() #1384

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

phorward
Copy link
Member

  • Introduces generic SkelModule.skel() function to obtain a skeleton instance.
  • Modifies all prototypes to accept bones_from_request

Replacement for #1376

@phorward phorward added the feature New feature or request label Jan 23, 2025
@phorward phorward added this to the ViUR-core v3.7 milestone Jan 23, 2025
@phorward phorward marked this pull request as ready for review January 24, 2025 15:56
@@ -37,7 +37,7 @@ def viewSkel(self, *args, **kwargs) -> SkeletonInstance:
:return: Returns a Skeleton instance for viewing an entry.
"""
return self.baseSkel(*args, **kwargs)
return self.skel(bones_from_request=True, **kwargs)
Copy link
Member

@sveneberth sveneberth Jan 24, 2025

Choose a reason for hiding this comment

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

  • *args get lost
  • bones_from_request cannot be controlled by calling viewSkel()? --> Please add in viewSkels signature and pass it.
        def viewSkel(self, *args, bones_from_request : bool=True, **kwargs) -> SkeletonInstance:
Suggested change
return self.skel(bones_from_request=True, **kwargs)
return self.skel(*args, **kwargs)

Copy link
Member Author

Choose a reason for hiding this comment

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

  • *args get lost

I want to get rid of this. It is a relic from days gone by and should be eliminated. To do it, we have to refactor all *Skel()-related stuff in prototypes, and it will also become a breaking change. Therefore, just throw it away for now here.

  • bones_from_request cannot be controlled by calling viewSkel()? --> Please add in viewSkels signature and pass it.

We can do it this, do you want it to overwrite this as a default behavior?

Copy link
Member

@sveneberth sveneberth left a comment

Choose a reason for hiding this comment

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

Now we have a header that controls a bonelist. The header is evaluated by default for all viewSkels.
But what happens if I load another skeleton within the same request? Then the same bonelist is also used for the other skel. You don't want that ...

Furthermore, the Vary header should be set in the response after (and if) the bonelist has been evaluated and applied.

src/viur/core/prototypes/skelmodule.py Outdated Show resolved Hide resolved
src/viur/core/prototypes/skelmodule.py Show resolved Hide resolved
@phorward phorward added the viur-meeting Issues to discuss in the next ViUR meeting label Jan 30, 2025
- Introduces generic `SkelModule.skel()` function to obtain a skeleton instance.
- Modifies all prototypes to accept `bones_from_request`

Replacement for viur-framework#1376
@phorward phorward force-pushed the feat-x-viur-bonelist branch from 7ec55ae to 32399e2 Compare February 3, 2025 19:38
akelch
akelch previously approved these changes Feb 5, 2025
@phorward phorward added Priority: High After critical issues are fixed, these should be dealt with before any further issues. and removed viur-meeting Issues to discuss in the next ViUR meeting labels Feb 5, 2025
- faces some review issues of @sveneberth
- no satisfying integration with existing code
@phorward phorward marked this pull request as draft February 5, 2025 22:59
@phorward phorward requested review from sveneberth and akelch February 5, 2025 23:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request Priority: High After critical issues are fixed, these should be dealt with before any further issues.
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

4 participants