Skip to content

Conversation

aaxelb
Copy link
Contributor

@aaxelb aaxelb commented Sep 10, 2025

main point is to improve in-browser experience of the api

  • when Accept header accepts html, give non-html responses (requested with acceptMediatype query param) wrapped in a minimal html <pre>
  • subjectively improve html-browse rendering used when header accepts html and acceptMediatype query param doesn't specify (don't worry too much about the details)
  • also some minor code cleanup, avoiding while True: antipattern (honestly irrelevant to the rest)

ENG-8779

@coveralls
Copy link

coveralls commented Sep 10, 2025

Coverage Status

coverage: 81.513% (-0.2%) from 81.693%
when pulling f6f285a on aaxelb:feat/8779-api-response-in-browser
into 8f08e7f on CenterForOpenScience:develop.

@aaxelb aaxelb force-pushed the feat/8779-api-response-in-browser branch 2 times, most recently from d5484cf to b1f2263 Compare September 10, 2025 18:43
- replace `trove.render._rendering` module with `trove.render.rendering`
  package (separate file for each rendering class)
- make `ProtoRendering` an actual `typing.Protocol` and narrow types to
  only unicode `str`
- add `trove.render.rendering.html_wrapped.HtmlWrappedRendering` that
  puts minimal `<!DOCTYPE html><pre>...</pre>` around an inner rendering
- use `HtmlWrappedRendering` to wrap responses that aren't html or json,
  when `Accept` header allows html and query params omit `withFileName`
- use mediatype constants more consistently (leaving off charset except
  for content-type header)
@aaxelb aaxelb force-pushed the feat/8779-api-response-in-browser branch 3 times, most recently from fb7ed0a to abc9985 Compare September 10, 2025 21:02
@aaxelb aaxelb force-pushed the feat/8779-api-response-in-browser branch from abc9985 to f6f285a Compare September 10, 2025 21:06
@aaxelb aaxelb requested a review from felliott September 10, 2025 21:09
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