[WIP] Add content negotiation to support text/markdown#2012
[WIP] Add content negotiation to support text/markdown#2012
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
Show files changed (1 files in total): 📝 1 modified | ➕ 0 added | ➖ 0 deleted
|
…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.
| 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.
| if url: | ||
| return f"" | ||
|
|
||
| # Return None for complex blocks |
There was a problem hiding this comment.
Could be made extensible using adapters
|
|
||
| # Fallback to JSON (default) | ||
| renderer = queryAdapter(self.request, IRenderer, name="application/json") | ||
| if renderer is not None: |
There was a problem hiding this comment.
This should never be None, since you registered one in this package. So I think the fallback below is unnecessary.
| # Wildcard - use default JSON | ||
| continue | ||
|
|
||
| renderer = queryAdapter(self.request, IRenderer, name=mime_type) |
There was a problem hiding this comment.
Maybe this should also adapt the service instance, so that it's possible to register a renderer for a specific service. (The markdown renderer here is registered for Interface, but is really only designed to work with the ContentGet service.)
|
|
||
| <plone:service | ||
| method="GET" | ||
| accept="application/json,text/markdown,*/*" |
There was a problem hiding this comment.
Does this actually do anything?
|
@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. :) |
3 participants
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/