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

Automated holiday generator #152

Merged
merged 8 commits into from
Mar 14, 2024

Conversation

tswistak
Copy link
Contributor

What is it?

  • Bugfix
  • Feature
  • Codebase improvement

Description of the changes in your PR

  • What has been done:
    • Node script based on date-holidays library for creating holidays for specific set of countries.
    • Everything is configured via objects in config.js. The only thing that can be set outside is an environment variable to set whether the script should write logs to the console.
    • Library supports 197 countries and regions, but I only chose 40 that are currently in the app. If needed, extending it is just modifying a variable with countries in config.js.
      • If we'd like to add region support, e.g. different states in the USA or Australia, we'd need to modify the script because I haven't considered this case.
    • GitHub action that creates pull request with updated holiday files. It should run automatically every 1st December (I hope it will work fine, I've never set cron running so rarely) or can be triggered manually.
  • What hasn't been done:
    • date-holidays unfortunately doesn't support some countries that already had holidays: India, Kazakhstan, Pakistan and Sri Lanka. I've added TODO comments with links to issues on library's GH to not forget about those.
    • I haven't used date-holidays-ical as I wrote in the original discussion because it had dependency to an old version of date-holidays. Instead, I decided to use a separate library for generating ICS files.
    • Current ICS files have fixed-date holidays set as always recurring. Unfortunately, from the library I could just get entry for each specific year, so I've added it like this, so e.g. if someone currently imported holidays with Christmas, they could see Christmas on 2030-12-24. Now it will only be visible in years for which ICS are generated.
      • If needed, it could be changed because date-holidays returns metadata with repeating rule. I could figure out from it which holidays have fixed dates. But the question is, whether we want to complicate that script this much.
  • What I'm unsure of:
    • I didn't know for what time period I should generate holidays. Now I've set generating for current and the next year. However, I don't see any problem with creating more holidays, since these are plain text files and they shouldn't consume too much space on user devices. However, I don't know what's other users' preference.
  • Additional remarks about implementation:
    • date-holidays returns different kinds of holidays. I've decided to filter them, to have only public and bank holidays, since I've noticed that some countries don't have other filled out. Also, for me, it would be confusing to have also other holidays, but I can easily change it (again, it's just a variable in config.js)
    • uid of each event is reproducible (sha-256 of country and date) to avoid duplicate entries after adding holidays in recurring years. I only hope that there isn't any country that has multiple holidays on the same date, so they don't get filtered out. I decided not to use the name of the holiday because some are localized, some not, so I don't consider the name as something constant.
    • I've tested GitHub action, but on a separate, private repository. I don't know why I couldn't run any workflow on a fork, so I think you just have to believe me it works.
    • Remark for JS devs: I know that we should use npm ci to install packages on CI and commit package-lock.json, but I specially omitted it, so the script always fetches the latest version of the library. I can only hope it won't end like faker and there won't be any major API changes between minor versions (there shouldn't, but who knows...).
    • Probably it would be nice to add somewhere information (readme, contribution guidelines, faq, wherever) that holidays are autogenerated, so they can have errors or missing localization. Maybe also with the link to the library's repository: https://github.com/commenthol/date-holidays, so they would raise problems or change localization there.
    • I've added some comments to the code so it can be easily understood by others, but if something will be wrong after merging, feel free to ping me.

Fixes the following issue(s)

Acknowledgement

@tswistak
Copy link
Contributor Author

There's a one thing that came into my mind - some countries have multiple languages, e.g. https://github.com/commenthol/date-holidays/blob/master/data/countries/BE.yaml. Now, script works in a way, that it takes the first language (e.g. here it will take French for Belgium). Would we like to support multiple languages, and if yes, how?

@naveensingh
Copy link
Member

naveensingh commented Mar 13, 2024

If needed, it could be changed because date-holidays returns metadata with repeating rule. I could figure out from it which holidays have fixed dates. But the question is, whether we want to complicate that script this much.

Google Calendar on Android doesn't seem to show fixed-date holidays for the next year but we already have some requests for this:

We should at least 'try' to parse the repetition rule for holidays with fixed dates.

date-holidays returns different kinds of holidays. I've decided to filter them, to have only public and bank holidays, since I've noticed that some countries don't have other filled out.

Ideally, we should import the other holidays and then allow filtering them out in the app itself but let's go with public and bank and we'll see how it works. Related feature request: #95

There's a one thing that came into my mind - some countries have multiple languages, e.g. https://github.com/commenthol/date-holidays/blob/master/data/countries/BE.yaml.

That's been requested as well: #167

Now, script works in a way, that it takes the first language (e.g. here it will take French for Belgium). Would we like to support multiple languages, and if yes, how?

I assume the first language is the local default for countries with holidays in multiple languages? If yes, let's go with that. I'll consider adding support for multiple languages/regions some day.

@tswistak
Copy link
Contributor Author

Thanks for the review! Later this week, I will write a parser for fixed-date holidays.

@naveensingh
Copy link
Member

Ok, thank you.

@tswistak
Copy link
Contributor Author

@naveensingh I've found some time today and added a parser for fixed date holidays. Here's a sample ICS file that the script now creates: unitedkingdom.ics.zip.

Some tech notes:

  • I've modified a bit how IDs are produced - now I also consider a repeating rule, not only the date, to ensure that fixed-date holidays won't duplicate in the app. It may also address the potential problem I described originally: I only hope that there isn't any country that has multiple holidays on the same date, so they don't get filtered out.
  • Fixed-date holidays are generated starting from 1970, but it can be easily modified in config file.

@naveensingh
Copy link
Member

@tswistak Thanks for the quick work and detailed changelogs! I'll check it out tomorrow (next 10-15hrs)...

@naveensingh
Copy link
Member

Everything else looks good to me! I'm ready to merge this if you are.

@tswistak
Copy link
Contributor Author

tswistak commented Mar 14, 2024

I'm also ready. I'll try to take a look sometimes if there are issues about holidays and, if needed, will fix or modify the generator. Just in case, feel free to ping me.

@naveensingh naveensingh merged commit 1a0ca55 into FossifyOrg:master Mar 14, 2024
@naveensingh
Copy link
Member

Ok, thanks :)

@tswistak
Copy link
Contributor Author

I've noticed that action has failed. I forgot to mention that you have to allow GitHub actions to create pull requests. You may find it in Actions ⇾ General ⇾ Workflow permissions

@naveensingh
Copy link
Member

Yep, allowed it. Now running it again...

@tswistak tswistak mentioned this pull request Apr 22, 2024
7 tasks
@curbengh curbengh mentioned this pull request Oct 12, 2024
4 tasks
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.

3 participants