matanshukry / flutter_google_places_sdk

Flutter plugin for google places native sdk
36 stars 74 forks source link

interface: Added types property to the autocomplete predictions #42

Closed mrcsilverfox closed 1 year ago

mrcsilverfox commented 1 year ago

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?

matanshukry commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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