-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
use API instead of in memory database #27
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggested change for the new API URL - also I think we can remove the ping code since we no longer have to worry about cold starts!
src/dataset.py
Outdated
@@ -28,7 +29,7 @@ | |||
client = Socrata("data.seattle.gov", None) | |||
LICENSE_DATASET = "enxu-fgzb" | |||
SALARY_DATASET = "2khk-5ukd" | |||
DATASET_PATH = Path(__file__).absolute().parent / "data" / "1-312-data.db" | |||
DATA_API_HOST = "https://spd-lookup.herokuapp.com" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that we have the API accessible on the new server, this can be changed!
DATA_API_HOST = "https://spd-lookup.herokuapp.com" | |
DATA_API_HOST = "https://1312api.tech-bloc-sea.dev" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! I'll merge it in as soon as I get an opportunity to test it locally 😄
Okay testing this a bit more, and I'm getting some odd results from the API: https://1312api.tech-bloc-sea.dev/officer/search?first_name=&last_name=Morris |
Ahh, okay, this has to do with your recent fuzzy search implementation! This is sort of unrelated to the PR, but is it possible to only do a fuzzy search if a certain character is added? For instance, Regardless, this PR looks solid to me and the application works! I'll merge it in tomorrow :) |
Yea right now, named searches default to fuzzy search. There is an api endpoint for exact match; we might want to think of the UX in terms of when to invoke fuzzy search vs exact match (maybe a checkbox instead of a |
Yeah I like the checkbox idea, since the API makes it easy to just switch between the two, and it gives users an easy to understand visual cue. Reduces how busy the window will be. Should probably default to fuzzy search. |
Got it! Yes, agreed with you both - I'll make a separate ticket for the fuzzy searching option. |
Opening a work in progress PR addressing #23
Uses API instead of in memory database for officer search.
Slightly changed search rules:
I realize this is a pretty big change, so opening this as more of a request for comment than anything else. Also, I realize that this creates a dependency on my project. I am more than happy to transfer ownership of that project to this group