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 Mauritius public holidays #279

Merged
merged 5 commits into from
Sep 22, 2023
Merged

Add Mauritius public holidays #279

merged 5 commits into from
Sep 22, 2023

Conversation

XSpielinbox
Copy link
Contributor

@XSpielinbox XSpielinbox commented Aug 24, 2023

🎁 Adds support for Mauritius
🐞 fix Cycle definitions for when they do not contain a validFrom definition. Previously it would just silently fall back to EVERY_YEAR behavior. Now it calculates backwards, if only a validTo is given and throw an exception if neither is given.

However, it is missing translations at the moment, as I did not know how to translate them.

Also as long as there is no support for the Tamil and/or Hindu Calendar (see #282), one has to manually define many holidays, which is very time consuming and error prone.

Additionally as long as there is no support for the Chinese Calendar (see #285), one has to manually define many holidays, which is very time consuming and error prone.

Please merge this PR AFTER #277.

@derTobsch
Copy link
Contributor

Good Morning @XSpielinbox, could you rebase your branch against the main? And maybe split the other pull requests #283 and #288 into branches? That would be easier to merge and maintain.

@XSpielinbox
Copy link
Contributor Author

Good Morning @XSpielinbox, could you rebase your branch against the main? And maybe split the other pull requests #283 and #288 into branches? That would be easier to merge and maintain.

Good morning @derTobsch, for sure I can rebase against main. I will have to see how to resolve the conflicts in the holiday_descriptions.properties.

What do you mean with "split the other pull requests [...] into branches"? I can rebase them against my respectively rebased branches for this PR and #283 or main again too, as they are on separate branches already, but I can only open pull-requests against existing branches in this repository.

@derTobsch
Copy link
Contributor

🐞 fix Cycle definitions for when they do not contain a validFrom definition. Previously it would just silently fall back to EVERY_YEAR behavior. Now it calculates backwards, if only a validTo is given and throw an exception if neither is given.

Can you give me an example?

And if both are not given, then it should not throw an exception, because this is legit if there was not starting or ending year of a public holiday.

@XSpielinbox
Copy link
Contributor Author

Can you give me an example?

I added a test case for that, see FixedParserTest.java.
A need for that can for example be seen in the Mauritius Holiday Id Al Fitr.

I noticed this shortcoming in the making of the previous holidays already (with that fix, some things in the already merged holidays could be solved more elegantly), but with Mauritius I saw no way around this anymore.

The issue is that in case of a "only every x years" cycle with only a validTo and no validFrom the limited.validFrom() != null would be false, therefore the if statement would not be taken and one would completely fall trough the switch statement hitting the return true; at the end of the method. This then results in the same behavior as if EVERY_YEAR would have been given, thus making it unusable.
When I have a holiday that is celebrated since e.g. 2012 and takes place every three years for example I can easily do that, but when it was celebrated at last 2012, but before that only every three years I cannot specify validFrom, because it was celebrated all along before that. But like I can calculate three years cycles starting from 2012 it is trivial to calculate three year cycles ending 2012. But when true is returned, every year before 2012 is regarded as a holiday, which is not what I want.

And if both are not given, then it should not throw an exception, because this is legit if there was not starting or ending year of a public holiday.

Yes, of course it is legitimate to have no start or end year. I did not change that. After all all the other holidays work as before and all the tests still pass. The problem is that for holidays that only occur every x years with x > 1, I don't know a way to determine which cycle is ment without any additional information. If I say every year, it is trivial. If I say every odd or every even year I also don't need a start or end date to say whether this year is in the cycle or not. But when I say every 4 years for example, how do I know whether the years should be ..., 2012, 2016, 2020, ... or ..., 2013, 2017, 2021, ... or 2014, 2018, 2022, ... or 2015, 2019, 2023, ...
An additional parameter is needed to determine it fully: I could for example say, every 4 years, including the year 2013. Then it would be clear that the second option must be ment.
When I say "every year dividable though 4", it is also exactly determined again: It must be the first of the 4 options, but the schema says every="FOUR_YEARS" and at least I would find it confusing, if it would assume that it assumes every year dividable though 4. To explicitly fail in this case (for which I don't see a use case anyway at the moment) and not silently fall back to something like EVERY_YEAR when I explicitly state FOUR_YEARS, just because I did not give a start or end date, I throw the exception. In case you have some good default for this case or have some amazing way to determine which possible cycle was ment, one can just replace the exception with that. Also if in some future additional support for example every though 7 dividable year would be added (for whatever reason), one could just replace the exception with handling for these additional cases. You can consider the Exception just as a place holder for now to avoid some (in my opinion not sane) default.

If I state EVERY_YEAR, ODD_YEARS or EVEN_YEARS, the code does not check the existence of validFrom or validTo as it did not before. After all it is fully determined, nevertheless and so now Exception will be thrown. Everything will just work as expected and just like before.

@derTobsch
Copy link
Contributor

@XSpielinbox can you translate it with https://www.deepl.com/translator for the other languages?

@derTobsch
Copy link
Contributor

Can you give me an example?

I added a test case for that, see FixedParserTest.java. A need for that can for example be seen in the Mauritius Holiday Id Al Fitr.

I noticed this shortcoming in the making of the previous holidays already (with that fix, some things in the already merged holidays could be solved more elegantly), but with Mauritius I saw no way around this anymore.

The issue is that in case of a "only every x years" cycle with only a validTo and no validFrom the limited.validFrom() != null would be false, therefore the if statement would not be taken and one would completely fall trough the switch statement hitting the return true; at the end of the method. This then results in the same behavior as if EVERY_YEAR would have been given, thus making it unusable. When I have a holiday that is celebrated since e.g. 2012 and takes place every three years for example I can easily do that, but when it was celebrated at last 2012, but before that only every three years I cannot specify validFrom, because it was celebrated all along before that. But like I can calculate three years cycles starting from 2012 it is trivial to calculate three year cycles ending 2012. But when true is returned, every year before 2012 is regarded as a holiday, which is not what I want.

And if both are not given, then it should not throw an exception, because this is legit if there was not starting or ending year of a public holiday.

Yes, of course it is legitimate to have no start or end year. I did not change that. After all all the other holidays work as before and all the tests still pass. The problem is that for holidays that only occur every x years with x > 1, I don't know a way to determine which cycle is ment without any additional information. If I say every year, it is trivial. If I say every odd or every even year I also don't need a start or end date to say whether this year is in the cycle or not. But when I say every 4 years for example, how do I know whether the years should be ..., 2012, 2016, 2020, ... or ..., 2013, 2017, 2021, ... or 2014, 2018, 2022, ... or 2015, 2019, 2023, ... An additional parameter is needed to determine it fully: I could for example say, every 4 years, including the year 2013. Then it would be clear that the second option must be ment. When I say "every year dividable though 4", it is also exactly determined again: It must be the first of the 4 options, but the schema says every="FOUR_YEARS" and at least I would find it confusing, if it would assume that it assumes every year dividable though 4. To explicitly fail in this case (for which I don't see a use case anyway at the moment) and not silently fall back to something like EVERY_YEAR when I explicitly state FOUR_YEARS, just because I did not give a start or end date, I throw the exception. In case you have some good default for this case or have some amazing way to determine which possible cycle was ment, one can just replace the exception with that. Also if in some future additional support for example every though 7 dividable year would be added (for whatever reason), one could just replace the exception with handling for these additional cases. You can consider the Exception just as a place holder for now to avoid some (in my opinion not sane) default.

If I state EVERY_YEAR, ODD_YEARS or EVEN_YEARS, the code does not check the existence of validFrom or validTo as it did not before. After all it is fully determined, nevertheless and so now Exception will be thrown. Everything will just work as expected and just like before.

Hey @XSpielinbox,
great work! It took me some time today to unterstand the code better. With your information and with the given code I totally understand what you mean and I like your implementation a lot.

I improved the code a little bit to clear things up and added tests for the ValidCycle class.

@derTobsch derTobsch merged commit 02f38fe into focus-shift:main Sep 22, 2023
9 checks passed
@XSpielinbox
Copy link
Contributor Author

@XSpielinbox can you translate it with https://www.deepl.com/translator for the other languages?

For sure, I can do that. However, you merged it already. Shall I just make a new PR for that?
Also: you already made some changes to the translations - what were your thoughts behind that?

@derTobsch
Copy link
Contributor

derTobsch commented Sep 22, 2023

@XSpielinbox can you translate it with https://www.deepl.com/translator for the other languages?

For sure, I can do that. However, you merged it already. Shall I just make a new PR for that? Also: you already made some changes to the translations - what were your thoughts behind that?

Hey, now I know what I forgot. I wanted to write you that the translations are done. I translated most of the new holidays. I need to look after the last translations, but that is not a big deal.

There where some issues with the encoding that I just fixed. So now that should be ok for the moment

@XSpielinbox
Copy link
Contributor Author

XSpielinbox commented Sep 22, 2023

Hey, now I know what I forgot. I wanted to write you that the translations are done. I translated most of the new holidays. I need to look after the last translations, but that is not a big deal.

There where some issues with the encoding that I just fixed. So now that should be ok for the moment

Ah, ok. Thank you. Yes, the German and Greek translations seem finished. I noticed a typo in the German translation however: It should be "Tag der Unabhängigkeit und der Republik".
There is also a typo in the javadoc of the new method in the ValidCycle.java.

The French, Dutch, Portuguese and Swedish translation are still completely missing, however. Shall I add them or will you do that too?

@derTobsch
Copy link
Contributor

derTobsch commented Sep 23, 2023

Hey, now I know what I forgot. I wanted to write you that the translations are done. I translated most of the new holidays. I need to look after the last translations, but that is not a big deal.
There where some issues with the encoding that I just fixed. So now that should be ok for the moment

Ah, ok. Thank you. Yes, the German and Greek translations seem finished. I noticed a typo in the German translation however: It should be "Tag der Unabhängigkeit und der Republik". There is also a typo in the javadoc of the new method in the ValidCycle.java.

The Dutch, Portuguese and Swedish translation are still completely missing, however. Shall I add them or will you do that too?

I will do that. I want to take a deeper look into the translations. And maybe provide some header for the utf8 encoding if that is possible

@XSpielinbox
Copy link
Contributor Author

I will do that. I want to take a deeper look into the translations. And maybe provide some header for the utf8 encoding if that is possible

Ok, but about the changes you made to the country_descriptions_nl.properties:
In GitHub it shows a change that makes sense: Graub�nden to Graubünden and Neuch�tel to Neuchâtel, but in IntellIJ IDEA on Fedora Linux it shows total nonsense: Graub�nden to Graubünden and Neuch�tel to Neuchâtel. In nano on Fedora Linux it shows the same change as on GitHub, however: Graub�nden to Graubünden and Neuch�tel to Neuchâtel - so something seems not right here.
Changing the IntelliJ IDEA setting Default encoding for properties files (located under File Encodings) to UTF-8 from the default of Default (ISO-8859-1) solves this issue, then everything get's displayed correctly everywhere.

file does identify all .properties files as Unicode text, UTF-8 text - so this does seem to be correct already.

An explenation that seems very relevant and good in this context: https://stackoverflow.com/questions/4659929/how-to-use-utf-8-in-resource-properties-with-resourcebundle

-> as far as I understand it, there is no such thing as a "header" for UTF-8 encoding in .properties files. For legacy reason an awful encoding is assumed by default in some tooling, especially old versions. Newer versions should default to UTF-8 and everything should be fine, but maybe one has to adjust some things to not use the legacy defaults (thereby I mean the IDE configuration or the java code that handles these files, though since Java 9 that should be fine too).

@XSpielinbox
Copy link
Contributor Author

@derTobsch Sorry, I by accident synced the wrong branch of my fork with upstream (which I just reverted) wherefore this PR is not successfully merged and closed anymore, but only closed.

I committed the typo fixes to this same branch. You may cherry-pick them or I can open a second PR. If you want I can also migrate them to a new branch and open a new PR from there.

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

Successfully merging this pull request may close these issues.

2 participants