freelawproject / doctor

A microservice for document conversion at scale
https://free.law/projects/doctor
BSD 2-Clause "Simplified" License
57 stars 15 forks source link

Improve Docker and Responses #133

Closed flooie closed 2 years ago

flooie commented 2 years ago

Drop m1 specific support

Drops many installs that are no longer required.

Adds support to return 406's for missing or required data in post requests

mlissner commented 2 years ago

Hm, I've got a few comments here. Might have been nice to review it before it merged, but can you clarify a few things and clean up a few other things, please?

  1. You removed the thumbnails view/url. How come? If we need to remove it, we should link up this commit with https://github.com/freelawproject/doctor/issues/126, and explain the situation.

    This also needs documentation updates.

  2. You've got some big try/except clauses. These always freak me out. With Django, why have these? Couldn't we just let the views crash? When they do, they'll send 500 errors, that we'll catch in CourtListener. If we do that, we can try to fix the 500's, knowing that the code crashed despite our attempts to throw 406 errors for bad input.

  3. Good to see the m1 and qpdf stuff going away. I'll try to carry this over to disclosures today.

  4. This needs a bit of polish in a few places. You've got a few docstrings that are empty """""". These should either go away or be completed, but not left as is. You've got code commented out, that should either go or be fixed.

  5. You want to be importing from HTTPStatus. Importing from http.client is just a backwards compatible shim. So:

    from http import HTTPStatus
    HTTPStatus.NOT_ACCEPTABLE

    (I got curious and poked in the code to figure out this one. Seemed strange to have two places to import it from.)

That's it for me for now. Guessing this is all just a relic of moving fast for our big deploy, but I wanted to do a review so we can get our standards back up now that we've got our mega deploy out the door.

Thank you!