Closed starcalibre closed 8 years ago
@starcalibre very cool. Somewhat implausibly, I made it through the whole thing thanks to your nice commit breakdown.
Once you've addressed the comments and added docstrings, we can get this in!
@jni Docstrings added and the database API views/routes have been refactored. Let me know what you think. I've had to add more routes to handle different requests, but every route is much more simplified. I'll add some docstrings if you agree on this new setup!
This actually eliminated the need for the InvalidParams
exception class. Any invalid routes now just return a 404 error.
@starcalibre this is great!
One last question: do we really want the api to be images/screen/image and samples/screen/sample? It makes more sense to me to have screen_id
be at the root of everything else. What do you think?
Sorry I'm not sure what you mean..?
If I want to get a particular sample out of the db, I GET from root/samples/screen_id/sample_id. I think it makes more sense to query root/screen_id/samples/sample_id.
Oh I see what you mean, so we'd have the following routes..
/screens
- Get all screens
/screens/<screen_id>
- Get a particular screen.
/<screen_id>/samples/<sample_id>/images
- Get an image
/<screen_id>/samples/<sample_id>/images/neighbours
- Get neighbouring images
/<screen_id>/features/<feature>
- Get a feature
/<screen_id>/samples
- Get all samples belonging to a screen
/<screen_id>/samples/<sample_id>
- Get a sample.
/<screen_id>/samples/<sample_id>/neighbours
- Get neighbouring samples
I like it because it conforms nicely to the SQL tables I've been working on (Screens have many samples and features, Samples has one image). I dislike it because it seems a little verbose but overall I think it's better. :)
Here, let me fix that for you: =P
/screens
- Get all screens
/screens/<screen_id>
- Get a particular screen.
/<screen_id>/images/<sample_id>/
- Get an image
/<screen_id>/images/<sample_id>/neighbours
- Get neighbouring images
/<screen_id>/features/<feature>
- Get a feature
/<screen_id>/samples
- Get all samples belonging to a screen
/<screen_id>/samples/<sample_id>
- Get a sample.
/<screen_id>/samples/<sample_id>/neighbours
- Get neighbouring samples
Does that work?
I prefer the images being being called from the samples path, because the images are part of the sample. I'd change the "Get neighbouring images" route to:
/<screen_id>/samples/<sample_id>/neighbours/images
- Get neighbouring images
That makes more sense to me, and it's consistent with the "get neighbouring samples" route.
Sure, that works fine for me!
@jni This should be good to go.
Note: I snuck in one more small update -- the flask-compress
plugin that uses gzip for compressing all responses.
@starcalibre just added a few more (minor) comments!
@jni LOL that picture is fantastic. Fixed the broken examples and comment.
Woot, in it goes! =)
Here its, the much awaited Flask backend! It's unavoidably a large PR. You can ignore anything in
app/static
andapp/templates
as those were mostly just moving files or making minor changes to AJAX requests to get them to play nicely with Flask.Flask has really nice client side templating built in by default. This lets you do cool things like define HTML programatically before it's sent to the browser. I'm going to the templating to simplify
app/static/index.html
in the next PR. I've left it out here to keep the size of this PR to a minimum.I can do a run-through of how Flask works if you'll be in tomorrow and have time? I plan to be at VLSCI.