Skip to content

Fix wrong fajr and sunrise time#2

Closed
azzamsa wants to merge 2 commits into
insha:masterfrom
azzamsa:myhack
Closed

Fix wrong fajr and sunrise time#2
azzamsa wants to merge 2 commits into
insha:masterfrom
azzamsa:myhack

Conversation

@azzamsa
Copy link
Copy Markdown

@azzamsa azzamsa commented Sep 26, 2020

- fix: `SolarTime::setting_hour` fails with value < 0
- fix: add missing HighLatitudeRule in prelude 

@insha insha added the bug Something isn't working label Oct 5, 2020
@insha
Copy link
Copy Markdown
Owner

insha commented Oct 5, 2020

Thank you so much for the pull request. Would you be able to add a test or two around this issue that is being addressed?

@azzamsa
Copy link
Copy Markdown
Author

azzamsa commented Oct 6, 2020

My PR mostly contains @RagibHasin works.

IMHO, I think It's better for Ragib to open A PR himself. Since he did a lot of improvement.

@RagibHasin
Copy link
Copy Markdown

Sorry for delayed response. I'm having problem with internet connection. I think I can open a PR tomorrow with the requested test cases included.

Thanks everyone.

@insha insha linked an issue Oct 10, 2020 that may be closed by this pull request
@insha
Copy link
Copy Markdown
Owner

insha commented Oct 10, 2020

@azzamsa and @RagibHasin I have reviewed the pull request. It is fine to go; but I would like to have the tests incorporated before it is merged. Let me know if this is possible and we will proceed accordingly.

Edit.1: I have reopened the original issue and linked to this pull request. Once the request is merged that issue will be closed out automatically.

Edit.2: I am stating the obvious and also just want to make sure that the issue is not that local and UTC times were being compared. Correct? Because the library only outputs results in UTC; which then will need to be converted to local time.

@RagibHasin
Copy link
Copy Markdown

@insha Actually the problem mainly revolves around UTC and local dates.
When at higher time zones (for me UTC+6) we make a date in UTC at Fajr time, It is still yesterday local and thus creates all the problem.

For instance, today Fajr time in Rajshahi, BD is 4:46 AM Local and Yesterday 10:46 PM UTC. This situation causes prayer.current() to fail and this failure cascades. But if we use Local times for public interface and UTC times for solar and other calculations as required, the problem vanishes.

Ideally I thought we should make this library generic over time zone for public API. But no time zone implements now() and today() except Local and Utc. So I think we are stuck to Local for now.

But beware, this would cause prayer.current() to fail if we are calculating for someplace other then home time zone. But I think then we would not use prayer.current(), would we?

My PR may come today or at most tomorrow, in sha Allah.

@azzamsa
Copy link
Copy Markdown
Author

azzamsa commented Oct 11, 2020

A little bit of story:

I am developing the CLI salah app on top of this. At first, it has 3 problems:

  1. I can't access HighLatitudeRule and solved by adding it to lib.rs
  2. I have the wrong farj time. This solved this fix by Ragib
  3. I and my user reported that the fajr time stuck and doesn't show the next prayer. This is solved by changing all UTC to Local. The root cause is because by using UTC all the prayer times happens on a different date. This is solved by Ragib in his uptosniff branch.

In uptosniff branch, Ragib solves the 3 mentioned issues and refactor the code to be more idiomatic. That PR is indeed huge, I don't have any idea if you can accept it.

@insha
Copy link
Copy Markdown
Owner

insha commented Oct 11, 2020

@azzamsa Please bear with me while I try to understand and work through each of the issues.

1. I can't access `HighLatitudeRule` and solved by adding it to `lib.rs`

This is fine.

2. I have the wrong farj time. This solved [this fix](https://github.com/insha/salah/pull/2/commits/89c234d67aee415cd50c8069aeffd491edd88ad2) by Ragib
3. I and my user reported that [the fajr time stuck](https://todo.sr.ht/~azzamsa/Bilal/1) and doesn't show the next prayer. This is solved by changing all `UTC` to `Local`. The root cause is because by using UTC all the prayer times happens on a different date. This is solved by Ragib in his [uptosniff](https://github.com/insha/salah/pull/3) branch.

This is interesting. The library is expecting a UTC date, therefore, when passing in the date for the calculation the client needs to take the local date and convert it to UTC before passing it on to the library. And once the result is received the client needs to convert the resulting times for each of the prayers to the desired local time from the provided UTC time.

The use of UTC is stated in the readme file, here, and here. The use of UTC is required because there is too much idiosyncrasies between time zones. UTC provides a consistent and stable date for all calculations. Therefore, I am of the opinion that the consumers of this library should be handling this complexity rather than the library itself. The main reason being that the handling of local time zone is dependent on the system that the client is targeting and supporting; thus it should be the client's responsibility to do the appropriate conversion.

That PR is indeed huge, I don't have any idea if you can accept it.

Indeed, this PR is large and is addressing multiple issues. It would be best to break this down into individual pull requests for each of the issues. If you haven't already please take a look at the contributing guide for this project.

@insha
Copy link
Copy Markdown
Owner

insha commented Jan 8, 2021

Closing this merge request due to inactivity.

@insha insha closed this Jan 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Wrong Fajr and Sunrise times are being reported for some time zones

3 participants