Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added a JewishDate constructor that takes sunset into account #193

Closed
wants to merge 2 commits into from
Closed

Added a JewishDate constructor that takes sunset into account #193

wants to merge 2 commits into from

Conversation

CompuGenius-Programs
Copy link

Seemed to be a big pain point for many users, including myself. Super easy to implement ourselves, but might as well have a method that does it for us.

Seemed to be a big pain point for many users, including myself. Super easy to implement ourselves, but might as well have a method that does it for us.
@CompuGenius-Programs CompuGenius-Programs changed the title Added a constructor that takes sunset into account Added a JewishDate constructor that takes sunset into account Nov 27, 2022
Copy link
Owner

@KosherJava KosherJava left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@compugenius ,
This seems a valuable change and I appreciate your effort. I have a number of comments.

  1. This change locks in sunset as the date change point, while some would want various tzais times for this. Why not pass a Date object as opposed to the ComplexZmanimCalendar (in theory some would be using the ZmanimCalender or even the AstronomicalCalendar for this, and you now force them to use this specific calendar), as well as force sunset (some may want to use sea level sunset and other elevation based). In short, changing to a Date would seem to make this simpler and much more flexible.
  2. The change forces you to reinstantiate /construct the JewishDate for every new day. What happens in scenarios like a user changing the date by the various setJewishDate() where an overridden version for these would allow passing a date as well to forward the date if it is after sunset/tzais to allow the HebrewDate to be one day forward.
  3. It would forcibly roll the Gregorian date ~6 hours early. This would potentially lead to issues.
  4. Your implementation of resetDateWithSunset() only works on the current date. What If I instantiated a JewishDate today for 1 Tishrei, calling this public method would be confusing to say the least. Yes, I know the JavaDocs mention this, but I think that many will find it confusing. You may want to think of a way to allow support of this valuable addition to work on other dates as well.
  5. I would also suggest that the resetDateWithSunset be renamed to something like forwardDateAtJewishStartTime() or something similar.

@CompuGenius-Programs
Copy link
Author

@KosherJava
I appreciate your feedback. To be honest, I did not think this far ahead haha, I just created a fix for an issue I had.

  1. Regarding passing a Date object, how does that help calculate sunset or tzais? Doesn't the framework need the coordinates of the user?
  2. As I mentioned above, I did not think too much into it haha. If I had spent some more time going through the framework, I would have changed the actual JewishDate calculation function, instead of just that one constructor. I had made a constructor so as to not change too much.
  3. I don't understand how changing a local instance of Calendar would cause issues.
  4. Well, the ResetDateWithSunset method was specifically for the current date and time, otherwise it's not really applicable, but as mentioned above, I would probably change the actual calculation function.
  5. Will do, although this function may be obsolete depending on if I can change the main calculation function.

I will say that I tried creating a copy of the file and editing it in Android Studio to test my changes, and I got a ton of errors, even after importing your package and changing the references.
Is there a better way to test it without having to download the entire project from GitHub?

@DanielSmith1239
Copy link

My idea would be to incorporate Gregorian hour/minute/second into the jewish date calculations throughout the JewishDate. The constructor could take a sunset time instead of a ComplexZmanimCalendar, which would allow for more freedom in choosing how the sunset is calculated (ex: it can default to midnight for when the user's location isn't available). I don't think it would be difficult to incorporate into the core JewishDate functionality, and it would allow for potentially removing workarounds throughout the rest of the codebase (like having different tefila rules for during the day vs maariv). I'm working on refactoring the Dart port of this library and I'm planning on making these changes to JewishDate there.

@CompuGenius-Programs
Copy link
Author

@DanielSmith1239,
Thanks for your suggestion! I don't have time (nor, to be honest, a need) to implement these changes into a PR at the time, but perhaps @KosherJava will do it, or I can in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants