chore: Migrate Place Info and Series Dates to V2 API#5959
chore: Migrate Place Info and Series Dates to V2 API#5959SandeepTuniki wants to merge 6 commits intomasterfrom
Conversation
Summary of ChangesHello @SandeepTuniki, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request updates the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
The pull request successfully migrates the get_place_info and get_series_dates functions from the V1 to the V2 API. This involved a significant refactoring of the logic to handle the new V2 response structures, including the introduction of helper functions _extract_place_info and _process_variable_dates. A new test file datacommons_v1_to_v2_test.py has been added to cover these changes, which is a great addition. The new implementations correctly process and aggregate data from the V2 API.
There was a problem hiding this comment.
I took a quick look -- Generally, these changes look good! It looks like these are the only references to v1/bulk/observation-dates/linked and v1/info/place, so let's also remove them from the endpoints list here:
website/server/services/discovery.py
Lines 132 to 156 in 4fe27a7
Feel free to mark this PR as "Ready for review" and ping me for re-review whenever you're ready and I'll take a more detailed pass!
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request successfully migrates get_place_info and get_series_dates from V1 to V2 APIs. The new implementations correctly handle the more complex logic required with the V2 APIs, such as making multiple API calls and performing local data aggregation. The addition of unit tests for these migrations is a great way to ensure correctness. The code is clean and well-structured. I have one minor suggestion to improve code clarity.
|
I addressed all the comments, but I see failing CI checks. I'll post here once I resolve them. |
(Created the PR to get feedback on the changes. I'm unsure if this is all changes I need to make).
This PR migrates two backend service functions, get_place_info and get_series_dates, from the V1 APIs to the V2 APIs.