[New Feature] - Adding Tel Aviv Stock Exchange calendar #4976
Conversation
…into JewishCalendar
There was a problem hiding this comment.
Hi @arnonm, thanks for the contribution. Overall it looks good. I have added a couple of smaller comments.
I have a local commit which removes some of the unnecessary changes (particularly to the project files). So feel free to comment and I can also apply changes on to my local branch.
One thing about the weekend: because the code runs very often, my idea was to cache the calculated holidays per year. But because the weekend is static, it is a set of days which is always checked always (usually Saturday and Sunday) and not cached per year.
From the code comments, I realized that maybe the weekend changed for the Tel Aviv stock exchange. It appears that starting 2026, trading is Monday through Friday, instead of Sunday through Thursday. Let's handle this in a separate pull request after this one is merged. We could add some specific logic in the TradeCalendar itself (or extend it).
| @Ignore | ||
| @Test | ||
| public void Israeli_Memorial_Day_should_be_a_holiday() | ||
| { | ||
| TradeCalendar calendar = TradeCalendarManager.getInstance("tlv"); | ||
| assertThat(calendar.isHoliday(LocalDate.parse("2024-05-13")), is(true)); | ||
| assertThat(calendar.isHoliday(LocalDate.parse("2025-04-30")), is(true)); | ||
| assertThat(calendar.isHoliday(LocalDate.parse("2026-04-21")), is(true)); | ||
| } |
There was a problem hiding this comment.
What is the reason to "@ignore" these tests?
I would prefer to either remove them (why test something that is not there?) or have a reason why this is ignored.
| @@ -273,6 +275,28 @@ public void post(String body) throws IOException | |||
| }); | |||
| } | |||
|
|
|||
| public String postReturn(String body) throws IOException | |||
There was a problem hiding this comment.
It appears that these changes are unrelated to the proposed trade calendar.
|
|
||
| // Tel Aviv Stock Exchange starting 2026 | ||
| // https://www.tase.co.il/en/content/knowledge_center/trading_vacation_schedule#vacations | ||
| tc = new TradeCalendar("tlv", Messages.LabelTradeCalendarTLV, STANDARD_WEEKEND); //$NON-NLS-1$ |
There was a problem hiding this comment.
What not use the TLV_WEEKEND here?
It is defined above, but then not used here.
I see the test differs between weekends "before 2016" and "after 2016"
| // JewishCalendarDate(JewishCalendarDate date) | ||
| // { | ||
| // this.day = date.getDay(); | ||
| // this.month = date.getMonth(); | ||
| // this.year = date.getYear(); | ||
| // } |
There was a problem hiding this comment.
Avoid commented out code. In this case, I propose to delete it. If needed later, we can add such a constructor
| return year; | ||
| } | ||
|
|
||
| @SuppressWarnings("unused") |
There was a problem hiding this comment.
Why suppress this warning? It is a private class. Delete the method (and we reduce noise)
| private int year; | ||
| } | ||
|
|
||
| private static class CalendarImpl |
There was a problem hiding this comment.
Maybe it makes sense to extract this class a package private class JewishCalendar in the same package. (Back when creating this class, I was a fan of private classes to keep the implementations hidden, but as the file is growing, then it could make sense to keep it separate).
| @@ -270,4 +805,31 @@ public Holiday getHoliday(int year) | |||
| } | |||
|
|
|||
| protected abstract Holiday doGetHoliday(int year); | |||
|
|
|||
| public JewishCalendarDate JewishHoliday(int georg_year, int jewish_month, int jewish_day, int add) | |||
There was a problem hiding this comment.
It is a method, so the first character those be lower case.
|
All valid comments, thank you.
The Memorial day issue is still a bug I will resolve it before we merge
Thanks for the suggestion on the weekend solution. For now, lets just keep
TLV on the regular weekend (as will be starting 2026). I can add
the alternating logic later
Agreed on all the other comments and will fix
Thanks for taking the time to review this.
|
|
All issues closed and last bug resolved. Ready to merge |
name.abuchen.portfolio/src/name/abuchen/portfolio/Messages.java
Outdated
Show resolved
Hide resolved
name.abuchen.portfolio/src/name/abuchen/portfolio/util/holiday-names_cs.properties
Outdated
Show resolved
Hide resolved
Nirus2000
force-pushed
the
JewishCalendar
branch
from
cbd69c3 to
de0372d
Compare
Fix typo Improve calender Add more test cases Sort Members in HolidayNames Sort holiday-names.properties
|
Hey @arnonm, thanks for the update. I am a little confused on the of the JewishCalendarDate class. For example, the method absoluteFromGregorianDate says it in its name that it converts a Gregorian date, but it gets passed a JewishCalendarDate. There are more such cases. I think for future readability, it makes sense to adopt this. /**
* Converts a Gregorian date to absolute day number.
*/
public int absoluteFromGregorianDate(JewishCalendarDate date)If you do not mind I will take your pull request and do some cosmetic changes here and there (such as the variable use, remove deprecated methods, etc). Give me a couple days as I am very busy at the moment. It reads a little bit that some ai generator converted the python code to Java... ;-) |
Nirus2000
force-pushed
the
JewishCalendar
branch
2 times, most recently
from
123ac87 to
bd868a0
Compare
|
No argument on the comment. I can fix it if you want |
|
I have a two more questions: First, can you look at the passover II non-trading days. (The other differences are - to my understanding - because PP shows the holiday because it is on a weekend - understood as Saturday/Sunday for the time being, and the TASE calendar does not always show them) 
Second, there is a |
|
Fixed the Passover II, thanks for catching that. Nice work on the commenting - always happy to learn. |
First PR for #4963
Adds a Tel-Aviv Stock Exchange calendar.
Since the TLV Stock Exchange uses Jewish holidays, added an implemention of Jewish holidays, addition of Holiday names
and a correction of a spelling mistake for the NIS - New Israeli Shekel (and not Sheqel in English)
Tests are added to check all the dates
Closes #4963