Skip to content

Update custom renderers to overload render#254

Open
rsamoilov wants to merge 1 commit intomainfrom
custom-renderers-update
Open

Update custom renderers to overload render#254
rsamoilov wants to merge 1 commit intomainfrom
custom-renderers-update

Conversation

@rsamoilov
Copy link
Copy Markdown
Member

This pull request introduces a redesign of the custom renderer system. Instead of generating separate render_<name> methods for each custom renderer, the system now overloads the main render method in controllers to support custom renderers via keyword arguments.

ref #244

@rsamoilov
Copy link
Copy Markdown
Member Author

Hey @anuj-pal27

I've been thinking about the render_* approach more, and after an extensive grilling session with Gemini I decided to update your implementation to overload the render method instead. The main argument is discoverability - seeing render something: true leads the developer to check the documentation for render and discover a link to the custom renderers functionality. Seeing render_something doesn't tell a developer not familiar with the framework anything.

Would love your review!

@rsamoilov rsamoilov force-pushed the custom-renderers-update branch from 085f4f3 to 7805151 Compare March 31, 2026 14:18
@anuj-pal27
Copy link
Copy Markdown
Contributor

Hey @rsamoilov — I reviewed the new approach of routing custom renderers through render (e.g. render csv: ...) and I understand the discoverability motivation, plus it keeps the status/body/double-render logic in one place.

One thing I want to double-check: if a renderer block sets headers["content-type"] (e.g. "text/html") and the implementation then delegates to render plain: result.to_s, does the plain branch still overwrite the content-type to "text/plain; charset=utf-8"?

@rsamoilov
Copy link
Copy Markdown
Member Author

No, it doesn't - the changes you've implemented are still there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants