Add new calculation methods #59
Conversation
|
👍 |
|
JazakAllahu khairun for this pull request. I do have a few comments however. Many of these calculation methods are the exact values of others. For example, france15 is the exact same calculation method as northAmerica. This appears to be an attempt to create more region specific methods, however I believe that is something that is better achieved through creating a feature to recommend a calculation method for a particular region. Or even possibly a method alias enum that does a many to one mapping to unique calculation methods. This keeps the calculation method enum smaller and more manageable. The potential recommended method feature could also encompass things like highLatitudeRule and madhab, to create a more simplified manner of specifying the parameters for a region. Also, I see you have added a ummAlQuraRamadan method, but this same effect can be achieved through the ummAlQura and setting the adjustments property. One reason I chose not to make a calculation method for this is that not all locations that use the ummAlQura have this change in Ramadan. As such, it is a much more case-by-case basis. For the egyptianGeneralAuthority and egyptianGeneralNewAuthority we should try to not change existing enums to avoid requiring people to update their code. We should also try to create a test file populated from an official government website in Egypt to validate what values they are actually currently using. Finally, in the case of the diyanet method, I do not think those values alone will give the correct times. There is an open issue discussing the diyanet method #23. If they are the correct times then we should add a json file to the "times" directory with values from the official website. I think your PR does offer some new methods that we are missing and we should definitely add them, but I want to avoid setting a precedent of adding methods with duplicate values. |
|
@z3bi va iyyak. Thank you for detailed answer! Totally agree. I'll use my dirty solution for now and will watch this repository for updates. |
No description provided.