Repository navigation
Conversation
|
@tisto thanks for creating this Pull Request and helping to improve Plone! TL;DR: Finish pushing changes, pass all other checks, then paste a comment: To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass, but it takes 30-60 min. Other CI checks are usually much faster and the Plone Jenkins resources are limited, so when done pushing changes and all other checks pass either start all Jenkins PR jobs yourself, or simply add the comment above in this PR to start all the jobs automatically. Happy hacking! |
Documentation build overview
7 files changed ·
|
…st of types, it needs to be application/json, when we return individual types, we return application/json+schema.
| @implementer(IRenderer) | ||
| @adapter(Interface) | ||
| class MarkdownRenderer: | ||
| """Render data as GitHub Flavored Markdown with YAML frontmatter.""" |
There was a problem hiding this comment.
Unlike the JSONRenderer, this is really only designed to render a particular type of data (a content item in the format produced by an ISerializeToJson adapter). That makes me wonder if you're trying to generalize too much by using this in the base Service class instead of only the ContentGet service.
Do we need to return a markdown representation from other services as well?
There was a problem hiding this comment.
That's a very good point. I am guessing that we only need the content endpoints to return markdown.
| body_parts.append(str(value)) | ||
| else: | ||
| # Everything else goes to frontmatter | ||
| frontmatter[key] = value |
There was a problem hiding this comment.
This would be more extensible if it was handled by field adapters rather than a hardcoded set of if/else statements based on hardcoded lists of fields. Of course, it's fine as a starting point for a proof of concept.
There was a problem hiding this comment.
I did not look at the Markdown generation code at all so far. I noticed that it does not look pretty though. Right now I am interested in looking at the architectural level. The details of the Markdown renderer is something that we can cover at a later point IMO.
There was a problem hiding this comment.
@davisagli I don't quite get how thats supposed to work, by adapting IFieldSerializer?
There was a problem hiding this comment.
@jnptk Not directly. IFieldSerializer adapters take care of converting the value of different types of fields into a representation that is ready to be serialized to JSON. But here we want markdown, and for each field we might want to add some things to the frontmatter and some to the body.
I guess we want a new interface IMarkdownFieldSerializer that is something like this:
class IMarkdownFieldSerializer:
"""Adapt a field to return markdown"""
def get_frontmatter(value) -> dict:
"""Return a dict to be added to frontmatter"""
def get_body_parts(value) -> list:
"""Return a list of markdown strings to be added to the body"""
There was a problem hiding this comment.
(This is brainstorming. We could certainly ship a first version of this that is not flexible using adapters.)
| return "\n".join(parts) | ||
|
|
||
| def _render_yaml(self, data, indent=0): | ||
| """Convert a Python dict to YAML format.""" |
There was a problem hiding this comment.
YAML serialization is hard to get right. For example, did you know that if the string "no" is serialized as no (unquoted), then it will be deserialized as boolean False?
So I would be inclined to use a battle-tested yaml library rather than roll our own.
There was a problem hiding this comment.
Good point. I am not fully sold on the YAML idea and I do not have much experience with using it on a programmatic level. Using a library is definitely something we should do if we decide on YAML.
There was a problem hiding this comment.
I replaced _render_yaml with https://pypi.org/project/PyYAML
|
|
||
| <plone:service | ||
| method="GET" | ||
| accept="application/json,text/markdown,*/*" |
There was a problem hiding this comment.
Does this actually do anything?
There was a problem hiding this comment.
yes, without this ContentGet service returns text/html, which makes me wonder how it worked before when there was no accept on the directive, am I missing something?
There was a problem hiding this comment.
I guess it is because of what plone.rest does based on this attribute for the directive.
|
@davisagli my goal with this PR is to have a PoC that allows me to raise questions regarding architectural decisions that we would have to make to add markdown support to plone.restapi. Therefore, comments are welcome, but please do treat this as what it is: a vibe-coded PoC that we most likely will throw away and start from scratch, once we have a clear picture of where we want to go. :) |
|
@jenkins-plone-org please run jobs |
|
Since this is WIP i converted it to a draft to mark this more prominently. |
If your pull request closes an open issue, include the exact text below, immediately followed by the issue number. When your pull request gets merged, then that issue will close automatically.
Closes #
📚 Documentation preview 📚: https://plonerestapi--2012.org.readthedocs.build/