Only show people with a photo as a guess. Fixes #37. #50
Conversation
There was a problem hiding this comment.
Thanks for fixing this, @blinry ! I was able to test it locally by clicking "next" a bunch and not seeing any "no photo" pictures; just to make sure the random number generator wasn't messing with me, I also removed a NOT from the query and saw only "no photo" pictures. Looks good!
There's a small, optional suggestion below; if you'd rather not change it, let me know, and I'll happily merge it as-is.
|
|
||
| hints = cursor.fetchall() | ||
|
|
||
| return person + hints |
There was a problem hiding this comment.
Does... fetchall just return a list now? That's so much better! This is much clearer than the silly [x for x in cursor.fetchall()].
| first_name, | ||
| middle_name, | ||
| last_name, | ||
| image_url |
There was a problem hiding this comment.
We talked about this in person, and I don't think it makes sense to fix now, but I want to note it here so I/we don't forget later:
For the hints, the only information we care about is the first_name; everything else is unused.
faces.py
Outdated
| ON people.person_id = stints.person_id | ||
| INNER JOIN user_batch | ||
| ON stints.batch_id = user_batch.batch_id | ||
| WHERE people.person_id != %s AND people.person_id != %s |
There was a problem hiding this comment.
We talked about this in person, too, and again it doesn't need to be fixed in this PR:
Now that we're doing two separate queries, and because these queries are each so long, I think it would make sense to extract a view. Possibly we'd also need to rename the existing batch_mates view, since that name might better describe this query.
Co-authored-by: Jaryn Colbert <jaryn.colbert@gmail.com>
blinry
force-pushed
the
filter-for-photo
branch
from
379a8dd
to
f24d260
Compare
|
I like your formatting suggestion, and I like the care you seem to give to your projects in general! 💚 I made the changes you suggested and force-updated this branch. Is that a reasonable workflow, or is there any GitHub feature related to reviewing which makes it more transparent what's going on? |
That's perfect; it's precisely the workflow I prefer! It keeps the history clean, and GitHub has been improving the UI around it over time, adding features like the force-pushed event in the PR history and indicating comments on changed lines are outdated. |
|
@jaryncolbert , would you like to merge this PR today? 😃 |
I would love to! Congrats, @blinry, and great work! 💛 |
No description provided.