gbdev / homebrewhub

A digital repository of of homebrews, patches, hackroms for old consoles. Provides community submission, tagging and rating features.
https://hh.gbdev.io
GNU General Public License v3.0
44 stars 9 forks source link

api: remove `.json` in the game manifest route #72

Closed dag7dev closed 2 years ago

avivace commented 2 years ago

Hey, I appreciate your effort, but this PR is quite confusing for me:

  1. We will not use Django to serve files or any other assets related to entries. Therefore, it makes no sense to expose a route that has to:
    • check if a file is actually there
    • handle the game.json in another URL
    • send the file through Django
  2. At some point, we may decide to enrich the Django model to accomodate more values from the game.json . In that case, we may want to serialize the Entry instead of sending the JSON from disk. Your change doesn't enable this as you are trying to merge the "serve_files" route with the one that should serve the game manifest
  3. The game attachments are already organized in the FS and accessible simply with the names exposed in the game manifest. Django doesn't need to serve those because it's enough to clone the database repository and tell Nginx (or whatever front web server) to serve content of the "database/entries/" folder under "entries". There's no need for an actual API route there..
  4. Last, but not least, the current deserializing - reserializing pipeline in the manifest route enables us to serve more data in the "game.json", simply by adding values in the dictionary (imagine values like "views" or "hits" that won't be saved in the game.json in the database repository)

In any case, each point mentioned here is up to discuss but we'd need to propose each change in a different PR, otherwise it'd be difficult to discuss and review the changes you are proposing

dag7dev commented 2 years ago

@avivace from README

GET /entry/<entry-slug>/<filename>

I thought that route wasn't implemented yet, therefore I thought it was missing :)