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

refactor: Move JSON-serializable rendering into the bones #1000

Draft
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

ArneGudermann
Copy link
Contributor

This PR moves the JSON rendering to the bones.
Resolves #978.

@ArneGudermann ArneGudermann added Priority: Medium This issue may be useful, and needs some attention. refactoring Pull requests that refactor code but do not change its behavior. python Pull requests that update Python code labels Dec 18, 2023
@ArneGudermann ArneGudermann added this to the ViUR-core v3.6 milestone Dec 18, 2023
phorward
phorward previously approved these changes Dec 19, 2023
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!
I'm a little bit unlucky with the naming regarding values and single_values... but this is a general problem in the current implementation that must be redesigned in the whole bone system.

For now, we can make it this way. Can you please add the suggestion to the VIUR4 issue #647, this is important to clean-up the interface, especially for newbies. Oops, this was related to the structure rendering, I'm sorry.

src/viur/core/bones/relational.py Show resolved Hide resolved
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.

Sorry, but this PR in this form makes no sense to me. The rendering of respectively for ALL renderers should be in the bones, not just for one. In addition, render_single_value should be passed the current renderer by argument, so that different outputs can be generated.

src/viur/core/render/json/default.py Outdated Show resolved Hide resolved
@phorward
Copy link
Member

phorward commented Dec 20, 2023

The rendering of respectively for ALL renderers should be in the bones, not just for one.

The intention of this PR is to turn a JSON-serializable rendering of values into the bones logic, for easier maintenance. It will be the base for further developments regarding JSON, XML and MsgPack renderers, because rendering stuff into JSON-serializable structures is heavily used in several situations.

The only special case is the HTML renderer for Jinja, which mostly just provides an adaption of the existing objects for faster access. This is a pull request on its own.

Indeed, this pull request doesn't address all cases of the concept, but this wasn't the goal.

In addition, render_single_value should be passed the current renderer by argument, so that different outputs can be generated.

This could be considered in a follow up on this pull request, but it should be optional. The rendering is separated from the renders here, so turning data into equal structures is the main target to achieve.

phorward
phorward previously approved these changes Dec 21, 2023
@phorward phorward changed the title refactor: Turn JSON-serialization into the Bones refactor: Move JSON-serializable rendering into the bones Jan 9, 2024
@phorward
Copy link
Member

@ArneGudermann please fix the conflicts with current develop.
@sveneberth would you like to discuss this in viur-meeting, or do you comply?

@phorward phorward added the viur-meeting Issues to discuss in the next ViUR meeting label Jan 17, 2024
@phorward phorward removed the viur-meeting Issues to discuss in the next ViUR meeting label Feb 2, 2024
phorward added a commit that referenced this pull request Feb 29, 2024
Proposal to fix #1064.

I'm still a little unhappy with this, as it bites with #1000, but after
all it's a different use case. Maybe we can bring both together in some
way.

---------

Co-authored-by: Sven Eberth <[email protected]>
@phorward
Copy link
Member

@ArneGudermann I would like to add this PR to viur-core 3.7, can you please resolve the conflicts and we can make another iteration over it?

@phorward phorward added Priority: High After critical issues are fixed, these should be dealt with before any further issues. and removed Priority: Medium This issue may be useful, and needs some attention. labels Jul 11, 2024
@phorward phorward removed the Priority: High After critical issues are fixed, these should be dealt with before any further issues. label Oct 14, 2024
@phorward phorward marked this pull request as draft October 16, 2024 06:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
python Pull requests that update Python code refactoring Pull requests that refactor code but do not change its behavior.
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

3 participants