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.

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

interface: Added types property to the autocomplete predictions #42

Conversation

mrcsilverfox
Copy link
Contributor

No description provided.

@mrcsilverfox mrcsilverfox force-pushed the interface-add-place-type-support-to-autocomplete-predictions branch 2 times, most recently from 02b23f8 to 2a42ff8 Compare May 16, 2023 15:00
@mrcsilverfox mrcsilverfox force-pushed the interface-add-place-type-support-to-autocomplete-predictions branch from 2a42ff8 to 4e210e2 Compare May 20, 2023 18:28
@mrcsilverfox
Copy link
Contributor Author

mrcsilverfox commented May 20, 2023

In a previous commit, some changes were made to the interface, (introduction of freezed package ).
toBusinessStatus method of the extensions BusinessStatus no longer exist,
FetchPlacePhotoResponse.imageUrl, does not accept an optional url

const factory Period({
    required TimeOfWeek open,
    required TimeOfWeek? close,
  }) = _Period;

open parameter is not nullable.

The tests failed because the toMap method is not found and Place has all required parameter, but in the previous test you use only LatLng.

What do you think about this? How we can solve these issues?

@mrcsilverfox mrcsilverfox force-pushed the interface-add-place-type-support-to-autocomplete-predictions branch from 4e210e2 to 0d07dbe Compare May 20, 2023 18:47
@matanshukry
Copy link
Owner

@mrcsilverfox the tests needs to be a redone, but that's ok - you can ignore them for now.

However the problem is that you're adding the placeTypes property to the response interface, and none of the platforms implement it.

Adding property to the implementations without the caller trying to parse it should not throw exceptions, so that should be the path forward; in each implementations (we have 4) verify we are returning the placeTypes, and after all 4 places are implemented and merged - we can change the response to include it and parse it.

iOS: https://github.com/matanshukry/flutter_google_places_sdk/blob/master/flutter_google_places_sdk_ios/ios/Classes/SwiftFlutterGooglePlacesSdkIosPlugin.swift#L331

Android: https://github.com/matanshukry/flutter_google_places_sdk/blob/master/flutter_google_places_sdk_android/android/src/main/kotlin/com/msh/flutter_google_places_sdk/FlutterGooglePlacesSdkPlugin.kt#L399

web: https://github.com/matanshukry/flutter_google_places_sdk/blob/master/flutter_google_places_sdk_web/lib/flutter_google_places_sdk_web.dart#L159

http: I think http is the only impl that might have it already actually, although it has a different name so might not work either.

@mrcsilverfox
Copy link
Contributor Author

mrcsilverfox commented May 21, 2023

Ok, so can I modify all platform plugin implementation in the same PR?
I have already done this on my personal branch with lots changes, but I think I can do it by editing as few files as possible.

If you want to look all changes needed, this is my fork commit

I had to comment some test, and refactor some classes.
Also I think that I had fixed #43.

@matanshukry
Copy link
Owner

@mrcsilverfox Each package that gets published needs to have it's own commit - although we don't have to wait for any of them.
The last one that uses them all need to wait for all of them,

Do note I'll check each one individually and will likely only be able to do it on the weekend.

@mrcsilverfox
Copy link
Contributor Author

Ok, but how can I "test" if each package works if a need to reference to an interface that does not exist?
This PR introduce the version 0.2.4+4. Each package will use this reference, but I cannot use it in my separate commit.
How can I do? This interface should be updated, I than I can ref to it on each package, or I misunderstood something?

@matanshukry matanshukry merged commit 8b649e3 into matanshukry:master May 25, 2023
@matanshukry
Copy link
Owner

@mrcsilverfox I belive I got confused, you're right. merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants