-
Notifications
You must be signed in to change notification settings - Fork 1
Clean unused json files and functions #346
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
Clean unused json files and functions #346
Conversation
| return endpoint_response | ||
|
|
||
|
|
||
| # /pdl/titles/title-i/subtitles/subtitle-a/map: |
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.
I believe these eliminated endpoints are now handled by other database endpoints, right? Can you confirm @sandeep-ps ?
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.
It's been removed for now.
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.
Yes, we are not using the /map endpoints in the frontend.
| return data_json | ||
|
|
||
|
|
||
| # /pdl/titles/title-ii/programs/csp/map |
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.
The removed endpoints need to be removed from pdl.yaml too.
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.
Thanks. It's removed.
|
Overall I don't see any issues created by removing the unused JSON files. Most were not used anywhere, a few still had API calls, but I don't see them being used on the frontend. My guess is they were replaced by another endpoint that returned similar data. |
app/controllers/data/title-ii/programs/eqip-ira/eqip_ira_state_distribution_min_max.json
Show resolved
Hide resolved
|
We can also remove the JSON files in the data/landingpage folder as those are also being served from the database. |
|
The rest of the JSON file removals and updating the YAML file looks good. |
|
I tested this PR by both running all the endpoints using Postmand and running the web app locally and briefly visiting most of the pages. Apart from the suggestion about the CHANGELOG entry, this looks good to me. I will also suggest that with the endpoints getting removed, and following the semantic versioning, the next API release would be 1.0.0. Thanks. |
…ttps://github.com/policy-design-lab/pdl-api into 345-task-clean-up-unused-json-file-reader-functions
sandeep-ps
left a comment
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.
Approving. Thanks.
I have tested the api PR with running frontend locally. Functions seem to be working.