search

batoulapps / Adhan

High precision prayer time library
MIT License
329 stars 63 forks source link

adding adhan-dart #97

Closed iamriajul closed 4 years ago

z3bi commented 4 years ago

MashAllah great job on this implementation!

One suggestion, I think it would be more correct to return a UTC datetime object. Then the developer would format the datetime object using a specific timezone for display. This is how the Java, Swift, and Javascript libraries work as well. Automatically converting to the local timezone will be incorrect if the location is not the user's current location, and using a fixed offset has many issues with changes to daylight savings time.

iamriajul commented 4 years ago

@z3bi dart has kind of weird timezone handling(can't customize timezone easily), that's why I thought it might be hard for devs to format again. then also there is a constructor/factory for UTC Timezone if someone want very own timezone customization (PrayerTimes.utc factory, using this will return UTC values). and also I think it's logical to return in device's timezone, people see clock in their devices.

Adhan-Java and when I was developing adhan-dart by following adhan-java then I noticed adhan-java also returns in Local Date Object. so that's also convinced me to write adhan-dart Local time default. image

Jazhaka Allahu Khairan.

iamriajul commented 4 years ago

and another thing is that adhan-dart's prayer calculations done using UTC time, then I have converted them when assigning properties at last by calling .toLocal() or .toUtc() (offset applied if passed) method. So there's shouldn't have any calculation errors for timezone.

iamriajul commented 4 years ago

and you might have also noticed that adhan-dart is 100% tests coverage with also tests data from Adhan repo.

z3bi commented 4 years ago

@iamriajul the tests look great! Is there no way though to make use of the variance variable here? That excludes some crucial test cases.

iamriajul commented 4 years ago

As I said previously timezone in Dart is Hard. and that's why I added the comment: // Couldn't Implement, Because Couldn't Parse Time in Different Timezone From String..

iamriajul commented 4 years ago

@z3bi brother I have implemented tests with variance just now. please check now.

z3bi commented 4 years ago

@iamriajul looks great!