Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Implemented Plugin for retrieving data from Google-Places-API #434

Merged
merged 25 commits into from
Nov 1, 2023
Merged

Implemented Plugin for retrieving data from Google-Places-API #434

merged 25 commits into from
Nov 1, 2023

Conversation

this-Aditya
Copy link
Member

  • Added GooglePlacesManager
  • Added GooglePlacesState
  • Added GooglePlacesProvider
  • Added GooglePlacesService

Fixes #432

@this-Aditya this-Aditya changed the title Google places Implemented Plugin for retrieving data from Google-Places-API Aug 16, 2023
blootsvoets
blootsvoets reviewed Aug 21, 2023
View reviewed changes
Copy link
Member

@blootsvoets blootsvoets left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Aditya, this looks very good already! I've made some comments mainly for readability and minor performance, but I see no structural issue. Good job!

...droid-google-places/src/main/java/org.radarbase.passive.google.places/GooglePlacesManager.kt Outdated Show resolved Hide resolved
...droid-google-places/src/main/java/org.radarbase.passive.google.places/GooglePlacesManager.kt Outdated Show resolved Hide resolved
...droid-google-places/src/main/java/org.radarbase.passive.google.places/GooglePlacesManager.kt Outdated Show resolved Hide resolved
...droid-google-places/src/main/java/org.radarbase.passive.google.places/GooglePlacesManager.kt Outdated Show resolved Hide resolved
...droid-google-places/src/main/java/org.radarbase.passive.google.places/GooglePlacesManager.kt Outdated Show resolved Hide resolved
...droid-google-places/src/main/java/org.radarbase.passive.google.places/GooglePlacesManager.kt Outdated Show resolved Hide resolved
...droid-google-places/src/main/java/org.radarbase.passive.google.places/GooglePlacesManager.kt Outdated Show resolved Hide resolved
plugins/radar-android-google-places/README.md Outdated Show resolved Hide resolved
plugins/radar-android-google-places/README.md Outdated Show resolved Hide resolved
@this-Aditya
Copy link
Member Author

Many thanks, @blootsvoets, for the review! I have updated the PR with the suggested changes.

@this-Aditya this-Aditya linked an issue Aug 22, 2023 that may be closed by this pull request
Create a new plugin for Google Places API. #432
Closed
@this-Aditya
Copy link
Member Author

Hi, I've made further updates to the disconnection logic, I guess this might be more effective than the previous one.

@this-Aditya
Copy link
Member Author

Updated!

blootsvoets
blootsvoets reviewed Sep 6, 2023
View reviewed changes
Copy link
Member

@blootsvoets blootsvoets left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks good. Just one more comment on when additional info is not enabled.

...droid-google-places/src/main/java/org.radarbase.passive.google.places/GooglePlacesManager.kt Outdated Show resolved Hide resolved
...droid-google-places/src/main/java/org.radarbase.passive.google.places/GooglePlacesManager.kt Outdated Show resolved Hide resolved
blootsvoets
blootsvoets approved these changes Sep 6, 2023
View reviewed changes
Copy link
Member

@blootsvoets blootsvoets left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for the changes

@this-Aditya
Copy link
Member Author

Hi @blootsvoets, one more thing: when using this plugin with GooglePlacesInfo.Builder(), it gives an error during the build process: Cannot access '<init>': it is private in 'Builder'. However, it works fine with GooglePlacesInfo.newBuilder(). Should I update it here as well?

@blootsvoets
Copy link
Member

Yes, good point. Use newBuilder as needed.

@this-Aditya this-Aditya mentioned this pull request Sep 20, 2023
Closed
@yatharthranjan
Copy link
Member

@this-Aditya the schemas are released now, can you include them so the CI passes. Thanks.

@this-Aditya
Copy link
Member Author

@yatharthranjan I've already created a PR for that -> #438

@yatharthranjan
Copy link
Member

ok i have merged that PR to dev, can you pull in the changes here so CI can pass?

@this-Aditya
Copy link
Member Author

Done!

@yatharthranjan yatharthranjan merged commit 45f723d into RADAR-base:dev Nov 1, 2023
2 checks passed
@this-Aditya this-Aditya deleted the google-places branch May 10, 2024 03:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@blootsvoets blootsvoets blootsvoets approved these changes

@yatharthranjan yatharthranjan yatharthranjan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Create a new plugin for Google Places API.
3 participants
@this-Aditya @blootsvoets @yatharthranjan