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: Add read method in RefSkel #1193

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from

Conversation

ArneGudermann
Copy link
Contributor

Resolves #1101

@ArneGudermann ArneGudermann added feature New feature or request Priority: Low This issue can be considered with enough idle time. labels Jun 21, 2024
@ArneGudermann ArneGudermann added this to the ViUR-core v3.7 milestone Jun 21, 2024
Copy link
Member

@phorward phorward left a comment

Choose a reason for hiding this comment

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

Hello @ArneGudermann, Thanks for facing this issue.
Can you first fix the provided suggestions?

Furthermore, I think it might be good to add a read-method generally to Skeleton, so that it can be used as a replacement for fromDB() - Why? Because issue #630 already proposes a Skeleton.read() method.

It should have the signature def read(self, key=None), and in the case that no key is provided, skel["key"] will be used (as it's the case here). If you want, I can make a follow-up pull request when this is merged.

src/viur/core/skeleton.py Outdated Show resolved Hide resolved
src/viur/core/skeleton.py Outdated Show resolved Hide resolved
src/viur/core/skeleton.py Outdated Show resolved Hide resolved
Co-authored-by: Jan Max Meyer <jmm@phorward.de>
src/viur/core/skeleton.py Outdated Show resolved Hide resolved
sveneberth
sveneberth previously approved these changes Jul 15, 2024
src/viur/core/skeleton.py Outdated Show resolved Hide resolved
Co-authored-by: Jan Max Meyer <jmm@phorward.de>
src/viur/core/skeleton.py Outdated Show resolved Hide resolved
@phorward phorward requested a review from sveneberth July 25, 2024 21:32
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: Low This issue can be considered with enough idle time.
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

None yet

3 participants