igneus / calendarium-romanum

liturgical calendar library (Roman Catholic, post-Vatican II)
50 stars 21 forks source link

Implemented time range querying #11

Closed simonszu closed 6 years ago

simonszu commented 6 years ago

I have implemented time range querying. Unfortunately, Ruby's Date::parse method only accepts full dates, speak: with year, month and day, so it is not possible to parse incomplete dates which specify time ranges. To solve this, i have created some switches, combined with regular expressions and tried to parse the given date by hand. I do not know what formats Date::parse accepts, so my regexps only accept YYYY-MM-DD and YYYY/MM/DD for now. If desired, i can add other formats with even more regular expressions.

For now, both acceptable formats are mentioned in the README, and the reference to the Date::parse method was removed from the README.

Furthermore, i fixed https://github.com/igneus/calendarium-romanum/issues/10. The solution you posted in this issue didn't work for me (at least with Ruby 2.4.2), so i solved it with a ternary operator. I hope it's okay.

And last, but not least, a change so small that i didn't want to create another branch for it: I added built gems to the gitignore. I don't know your workflow, but i used gem build and gem install every time i wanted to test my code manually, which resulted in a built gem inside the work dir, which one could accidentally commit when using git add ., so i excluded it in the gitignore.

igneus commented 6 years ago

Tip: when manually testing the executable, I don't build and install the gem (except when testing if the gem builds and installs correctly), but execute it in a more straightforward way: ruby -I ./lib bin/calendariumrom ...

simonszu commented 6 years ago

So, as you can see, i have written a new class for parsing the date ranges, and discovered that i can use the other range classes specified in CalendariumRomanum::Util. This works for now.

But now i am struggling with the calendar format. As you can see in my [WIP] commit, i try to use one PerpetualCalendar creating one Calendar for each time range, and providing this Calendar to the print_single_date method. However, there is the classical error i am making with the difference Liturgical year <=> Civil year.

Can you give me a hint, please?

Hm - or, wait. I didn't see the "if you need them" comment in the README-section for the PerpetualCalendar. I'll continue fiddling with this stuff...

simonszu commented 6 years ago

Well, i think i figured it out. My mistake was, that i created a PerpetualCalendar instance, and then a Calendar instance from it. Of course, this did not work. After deleting the creation of the Calendar instance, it worked.

So, now we can debate if it is clean enough code when the parser either creates timerange-enumerator-objects, or just a single date, and the query command switching these objects afterwards - but the only workaround for this would be some kind of single-day enumerator object, which also responds to the .each command, but i think this is ugly as well.

simonszu commented 6 years ago

Well...seems, at least, i still do need some help with calendars. As you see, i have used PerpetualCalendar. Turns out, PerpetualCalendar does not return any celebrations. So, i need to create a new liturgical calendar object for each day? But then i am at the very beginning again, with creating lots of liturgical calendar objects, which is not desired.

Where's my mistake? How would you solve it?

igneus commented 6 years ago

but the only workaround for this would be some kind of single-day enumerator object, which also responds to the .each command, but i think this is ugly as well.

So do I. In this case I would not use Util::Year and ::Month, but simply a Range of Dates. It is possible to make a Range of only one value, e.g. 1 .. 1.

igneus commented 6 years ago

Turns out, PerpetualCalendar does not return any celebrations.

It does return celebrations, but no sanctorale celebrations. The reason is that

pcal = PerpetualCalendar.new

does not provide the newly created instance with any sanctorale data. Change to

pcal = PerpetualCalendar.new sanctorale: sanctorale
simonszu commented 6 years ago

Okay, works now. I also have added the coverage/ folder created by rspec to the gitignore.

simonszu commented 6 years ago

I have implemented your annotations, and also created some unit tests for DateParser

igneus commented 6 years ago

Thank you, code is ready to be merged now. I will ask you for a last single change: please, undelete tmp/aruba/.keep and delete tmp/aruba/*.txt. The files are deleted/created by aruba while executing tests and no changes in this directory should ever be committed.

It would also be useful to rename spec/util_spec.rb to spec/date_parser_spec.rb. (I plan to split files containing multiple classes to one-class-per-file in the near future.)

simonszu commented 6 years ago

No problem. I thought i already had re-added the keep-file, but as usual, git add . does not add hidden files ;)

igneus commented 6 years ago

Btw. I noticed you rebase and force-push (or do something else to the same effect) after each review and code modification. That's not necessary. Sometimes I may ask for squash at the end of the review cycle, especially in cases of "history pollution" (unnecessary merges of an upstream branch to the local branch and similar) or excessive trial-and-error commit sequence. But usually even this "final squash" is not necessary.