Add an object for holiday descriptions

[YizYah]

I think the desc entries in staticHolidays should be exposed as an object. The desc strings are currently brittle, which is a problem for someone who wants to process events in a calendar based on the type of day.

For instance, now we could say:

import { holidayDescs } from '@hebcal/core'
...
if (event.desc===holidayDescs.EREV_SUKKOT) {...}

I considered calling them specialDayNames, but I figured I'd just stick as closely as I could to your current naming conventions.

If you like, I could update the README with an example.

[mjradwin]

Thanks for this - it's a great idea, but I'm seeing some unit tests fail, so it requires more investigation and can't just be merged in immediately.

[sonarcloud[bot]]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

[mjradwin]

I do like this idea, but I must confess that I'm a bit nervous about this change for a few reasons:

1. For example, you introduced an unintentional chul: true for Family Day, which is only observed in Israel. Our existing unit tests were able to catch this error, but I'm curious why you sent a PR without running the unit tests first?

2. The change only addresses strings in staticHolidays.js but not the dynamic holiday names in holidays.js. Was this an oversight? If you really want a complete list of holiday strings, we would want to modify the getHolidaysForYear_() function too.

3. How would you expect and end-user to handle "Tish'a B'Av (observed)" if they were only expecting "Tish'a B'Av"? The holiday description could be either.

4. Lastly, although it's very minor, I don't love the whitespace reformatting in commits like this https://github.com/YizYah/hebcal-es6/commit/62c5a307b589b237182b4ffa60237e27dda4f306 The whitespace change is contrary to the eslint configuration we use which prefers no whitespace around the curly braces on import statements. That change touches every single line in the index.js file, which can of course be reverted in a subsequent commit, but would make future "git blame" a bit more difficult.

[mjradwin]

Thanks again for the great suggestion! We have taken your idea but implemented with a script to avoid introducing errors. Take a look at @hebcal/core version 5.3.9 and let us know if this works for you.

Closing this pull request as it's incompatible with the way we implemented the feature.

[YizYah]

@mjradwin Thanks for reviewing my code, and for your accurate objections. And also thanks for the rapid turnaround, as before!

The truth is that I was a bit confused about the contribution process. I did not see any documentation, and actually I didn't think you were running the tests with each build, because when I ran them on your main branch I got 38 failures. I still do!

I figured that you would have incorporated whatever tests you had into your CI, which passed. Perhaps they should be added there as an extra safeguard? (Not that I disagree about testing before pushing, should have been done.)

Regarding the white space, I was also confused about why it happened. I thought VS code would automatically use the linting of the project. Apparently that's not true. Perhaps there's an error in my VS code configuration. But I did run npm run lint just now and it caught 1500 issues--that's the config that you're using, right?

Regarding not updating the holidays file, I must admit that I did not see how all of the code fit together. I still am taking in the holidays file. I found staticHolidays, and figured that was the "source of truth" for the descs. I did not think you would have referred to the same magical string in two locations, and it did not occur to me that the staticHolidays were incomplete.

What might be super helpful would be a section on contributing in the README. Maybe also a general overview about the architecture.

At any rate, do you have any idea why my unit tests are failing? Because I saw some light changes just now that I might propose, but I can't do that until I can pass the tests.

I ran npm i and npm run build before running npm test.

[mjradwin]

Thanks for the follow-up here. We'd be delighted to have you as a contributor and help get your environment set up.

I'm surprised that you're getting 38 unit test failures on the main branch. If you could share the output here, it might help us to figure out what's wrong with the build config or your environment? As part of GitHub Actions CI, we run unit tests on every checkin and PR for 4 major version of Node.js (16.x, 18.x, 20.x, 22.x) - and the unit tests are succeeding there in general:

https://github.com/hebcal/hebcal-es6/actions/workflows/node.js.yml

Yes, it looks like our lint configuration isn't correctly ignoring src/*.po.js files, which is what is generating the 1500 lint errors. We'll get that fixed momentarily.

[YizYah]

@mjradwin output from testing 5.3.9 is here. But with 5.3.10 I get the same results.

I was running node v21.7.1. But, I upgraded to v22.2.0 and am getting the same results. npm version 10.7.0.

I see that the linting is now fixed.

[mjradwin]

Thanks for posting the output. The dates are all off-by-one. It looks like a timezone problem, because I can reproduce the error by running TZ=Asia/Jerusalem npm test

In theory our unit tests should work correctly in any timezone, but clearly they don't. I'll work on fixing this.

In the meantime, can you try running TZ=UTC npm test and confirm that the unit tests pass?

[mjradwin]

OK, we have fixed unit tests for computers with Israel timezone and have pushed changes to main branch. It wasn't a very difficult fix. Take a look and let us know if they are working for you now?

Shabbat shalom!

[YizYah]

@mjradwin Yes, works now! 👍

[YizYah]

@mjradwin I've decided to start small with a simple restructuring. Please check out https://github.com/hebcal/hebcal-es6/pull/438. If it's not to your liking, I won't be offended! You know the old expression (paraphrased): two coders, three opinions! And I respect your work here. I only want to help if you are completely comfortable with it.

I am working on my vs code config. For some reason, I have to run npm run lint -- --fix to remove my linting. I made sure to do that, and the tests are passing.