[Kde-pim] Review Request: KHolidays - Add alternative calendar systems support - Preliminary review

[Allen Winter]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/2647/#review3753
-----------------------------------------------------------

Ship it!


awesome.

- Allen


On 2010-01-19 01:37:35, John Layt wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/2647/
> -----------------------------------------------------------
> 
> (Updated 2010-01-19 01:37:35)
> 
> 
> Review request for KDE PIM.
> 
> 
> Summary
> -------
> 
> This change is a preliminary step to adding support to kholidays for alternative calendar systems by extending the existing file format and parser.  For example, a holiday file could include Islamic holidays defined in the Hijri calendar:
> 
> 	"New Years Day" red on 1/1
> 	"Christmas Day" red gregorian on december 25
> 	"Muharram" red hijri on 1/1
> 	"Eid ul-Fitr" red hijri on shawwal 1
> 
> Which for 2010 will generate the following holidays:
> 
> 	2010-01-01  "New Years Day"
> 	2010-09-10  "Eid ul-Fitr"
> 	2010-12-08  "Muharram"
> 	2010-12-20  "Christmas Day"
> 
> This is a request for a preliminary review of the file parser as, while the api changes are not completed yet, this is about the last step when the data structure output from the new parser can be exactly matched to the data structure of the old parser and so makes a suitable checkpoint to demonstrate that the new parsing and calculation code is correct.  Once integrated it will be resubmitted for a full review.
> 
> I'll send a separate e-mail to the list to discuss my strategy and other proposed changes.
> 
> You can test this without affecting KHoliday or the current parser by applying the patch and running in kdepimlibs/kholidays:
>     bison -p kcal2 -d -o parseholiday2.cpp parseholiday2.ypp
>     flex -Pkcal2 -i -B -oscanholiday2.c scanholiday2.lex
>     cmakekde
> Then run "tests/testparseholiday -maxwarning 0".  You can tweak the output to print all the holidays compares if you want but it's rather long :-)
> 
> Note the patch is as new files, as this allows for parallel testing, and so much has changed a diff would be too messy to follow.
> 
> The new parser has 2 main talking points:
> 
> 1) It compiles as C++ rather than C, allowing it to use KCalendarSystem.  It does not use the Bison C++ skeleton to do this as that made my head implode. Instead it just names the file .ypp to force compilation as C++.  There's probably more to do here to make this as clean as possible.
> 
> 2) The parser reads the file multiple times, once for each calendar system and year in that calendar system that overlaps the required date range.  This means the file is currently read between 15 and  25 times to produce a single result year, but this is fairly fast given the small file sizes.  I'm trying to find a way do do this in a single pass, it may be possible by working directly with the parse tree and repeatedly evaluating a sub-branch, but could be tricky and not worth it.
> 
> I do have an interim version in pure C if that is preferred, but that entailed copying all the calendar calculation code in from KCalendarSystem, not a desirable thing.
> 
> 
> Diffs
> -----
> 
>   trunk/KDE/kdepimlibs/kholidays/CMakeLists.txt 1075911 
>   trunk/KDE/kdepimlibs/kholidays/holidays/holiday_gb 1075911 
>   trunk/KDE/kdepimlibs/kholidays/parseholiday2.ypp PRE-CREATION 
>   trunk/KDE/kdepimlibs/kholidays/scanholiday2.lex PRE-CREATION 
>   trunk/KDE/kdepimlibs/kholidays/tests/holiday_test PRE-CREATION 
>   trunk/KDE/kdepimlibs/kholidays/tests/testparseholiday.h PRE-CREATION 
>   trunk/KDE/kdepimlibs/kholidays/tests/testparseholiday.cpp PRE-CREATION 
> 
> Diff: http://reviewboard.kde.org/r/2647/diff
> 
> 
> Testing
> -------
> 
> The output of the new parser has been compared to the output of the old parser for a period of +/- 30 years (see testparseholiday) and they exactly match, except for 3 differences:
> 
> 1) Two digit years were accepted by the old parser, the new parser requires four digit years to prevent ambiguity.
> 
> 2) The old parser incorrectly calculates the extended relative weekday syntax e.g. [last monday in may], but calculates standard relative weekday syntax correctly.  The new parser fixes this.
> 
> 3) The old parser does not check the validity of file input dates and will create an event on an incorrect date instead. The new parser does validity checking and will skip invalid dates.
> 
> (The incorrect holiday files have been fixed)
> 
> Obviously there is no guarantee this exercises all possible cases, but is at least assurance that all currently required cases work.
> 
> 
> Thanks,
> 
> John
> 
>

_______________________________________________
KDE PIM mailing list kde-pim at kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/