batoulapps / adhan-swift

High precision Islamic prayer time library for Swift
MIT License
185 stars 46 forks source link

Add recommended calculation method #72

Closed basememara closed 1 year ago

basememara commented 2 years ago

Map time zone identifier to known calculation methods. Using the United States time zones identified at Wikipedia and left off the deprecated ones.

Further discussion in #41.

z3bi commented 2 years ago

@basememara this is an interesting idea. I think ultimately @iTarek is correct and country code to calculation method is going to be the most reliable. I liked your idea of using the CLPlacemark as the primary method. Maybe we could put together something like this


public extension CalculationParameters {
    /// Returns the recommended Calculation Method for the specified geographic data.
    static func recommended(for placemark: CLPlacemark) -> CalculationParameters? {
        let countryCode = placemark.isoCountryCode
        return recommended(forCountryCode: countryCode)
    }

    static func recommended(forCountryCode: String) -> CalculationParameters? {
        ...
    }
z3bi commented 2 years ago

I also think the easiest way to organize the data is to invert it, like this

let defaultMethods: [Method : [String]] = [
                .egyptian : ["EG", "SD", "SS", "LY", "DZ", "LB", "SY", "IL", "MA", "PS", "IQ", "TR"],
                .karachi : ["PK", "IN", "BD", "AF", "JO"],
                .ummAlQurra : ["SA"],
                .dubai : ["AE"],
                .moonsightingCommittee : ["US", "CA", "UK", "GB"],
                .kuwait : ["KW"],
                .qatar : ["BH", "OM", "YE", "QA"],
                .singapore : ["SG", "ID", "MY"],
                .tehran : ["IR"]
            ]
basememara commented 2 years ago

Just to note this is easier to maintain the code, but the lookups are from the country perspective, and will be hard to prevent duplicates (country in multiple methods).

basememara commented 2 years ago

Regarding using countries over time zones, I agree it's more accurate. Though it's more difficult to query the country of a user than the time zone, since geocode has API quota limitations and also requires internet and a GPS location. The time zone is synchronous and could be retrieved from the device offline. This probably shouldn't be a concern for the Adhan library, but are concerns from the caller's perspective.

z3bi commented 2 years ago

@basememara it might be worth it if you wanted to put together a separate library that converts TimeZone to Country code. I could see that being useful if someone is unable to use CLGeocoder and would potentially benefit other uses outside of Adhan. But I think trying to tie timezone to a calculation method feels like it's potentially adding a slew of concerns that Adhan shouldn't be trying to deal with.

basememara commented 2 years ago

I updated it based on country and moved to CalculationParameters extension. I used @iTarek's JSON file that was provided, only think I moved was "US" to .northAmerica.

A couple of thoughts is I was thinking of moving this to a separate file called CalculationParameters+Recommended, but wasn't sure if extra files would be preferred. Also this would work fine as a CalculationMethod extension, but thought would be more flexible in CalculationParameters to further customize the parameters. Let me know if you'd like any changes to any of the logic or convention.

What do you think?

z3bi commented 2 years ago

this data is something we would want to easily make available not just in the Swift repo but in all the other languages as well. There might be a better way to store this is data and then keep it in a shared repo instead of maintaining it as code in each repo.

basememara commented 2 years ago

I moved the country mappings to an embedded JSON file within the Swift Package, but maybe as a next step to figure out how to put it in the shared repo and adjust the scripts to pull it properly.

Also having trouble with the tests compiling because using Swift Concurrency in the test. Couldn't figure out how to bump the macOS version for fastlane tests, figuring this would be a good hump to overcome so we can start using Concurrency at some point. Any ideas or should I remove the Concurrency from the test?

basememara commented 2 years ago

..also wasn't sure if I should store the decoded data into a static property so it wouldn't have to decode again, or do it every time so it doesn't linger in memory for no reason. I leaned towards the latter because this doesn't seem like a function that will be run often but only specific scenarios like setting changes on the caller side.

z3bi commented 2 years ago

@basememara I also agree with not storing the list statically. Concurrency on the tests is nice, but I think the build is failing because Swift 5.3 is needed for accessing resources in a swift package so we will need to remove the swift 4 builds.

basememara commented 2 years ago

I removed Swift 4 from fastlane but still complains about not recognizing the Bundle.module, not sure where else this should be done. Also earlier it was complaining the Concurrency is only support on macOS 10.15+, so I also removed Concurrency from the tests as well. It looks like macOS is where the CI is still failing.