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

Add Torah Reading event flag #6

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

chabad360
Copy link

@chabad360 chabad360 commented Jun 8, 2023

This pr adds a flag TORAH_READING. Admittedly, this code is a little unclear to me, so the implementation is probably incomplete and I've probably missed a few spots or did something in a way that isn't in the same pattern as this library.

Gonna leave this as a draft until it's confirmed to be good. Fixes #5

@mjradwin mjradwin marked this pull request as ready for review June 8, 2023 19:27
@sonarcloud
Copy link

sonarcloud bot commented Oct 16, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@dsadinoff
Copy link
Member

dsadinoff commented Oct 17, 2023

Not sure why Leil Selichot has torah reading.

@chabad360
Copy link
Author

Because it's on shabbos (unless I understood the code wrong)

@dsadinoff
Copy link
Member

A separate question: do events like Shabbat Chazon need a torah flag? They don't actually affect torah reading at all, but rather the haftarah. Special haftarah feels like a different indicator.

Proposal: modify "TORAH_READING" to indicate that the main laining changes or that an additional sefer torah is required.
( Nobody is proposing that we add TORAH_READING to every Monday, Thursday and Saturday)
SPECIAL_HAFTARAH might be a good idea for things like shabbat chazon, but that will be a much messier category with fuzzier boundaries, and will be, I think, harder to pin down actual practice, since it varies.

@dsadinoff
Copy link
Member

Because it's on shabbos (unless I understood the code wrong)

nobody says selichot on shabbat, they say it on saturday night.

@chabad360
Copy link
Author

chabad360 commented Oct 17, 2023

Nobody is proposing that we add TORAH_READING to every Monday, Thursday and Saturday

Actually, it's in the PR.

nobody says selichot on shabbat, they say it on saturday night.

True. But the event is marked for a certain day, not a time (un/fortunately), and so the flag still applies. If you're worried about duplicates, the API already returns multiple events for a given day in certain instances, so one would need to dedup by date anyway. Or perhaps the module should be adjusted to only return the requested flags for a given day, but that's another conversation.

The intent of this PR is to flag every instance that the Torah is read (this was something I had to do by hand for a previous project with this module, which is why I proposed it). Other markers for a (special) haftorah being read could be great too.

@dsadinoff
Copy link
Member

True. But the event is marked for a certain day, not a time (un/fortunately), and so the flag still applies. If you're worried about duplicates, the API already returns multiple events for a given day in certain instances, so one would need to dedup by date anyway. Or perhaps the module should be adjusted to only return the requested flags for a given day, but that's another conversation.

Perhaps I don't understand how this flag is to be used. Can you describe what would be a real-world negative consequence of not having the flag on the leil selichot event? I'm asking because it seems like semantic harm is being done when storing false data ( "Leil selichot needs a torah").

@mjradwin
Copy link
Member

For what it's worth, I found that maintaining the special leyning rules over the last few years in the JavaScript version of Hebcal was complex enough that it was worth splitting out into a separate package.

If there is a need for things like special Haftarot and maftir and such in Golang, we could consider porting https://github.com/hebcal/hebcal-leyning from JS to Golang.

Note there is also a REST version of that leyning API

https://www.hebcal.com/home/4277/leyning-torah-reading-api

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.

Add an event flag for torah readings
3 participants