igneus / calendarium-romanum

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

to_s implementation for season and colour #20

Closed Oneill38 closed 6 years ago

Oneill38 commented 6 years ago

Fixes #17

In response to your question, yes the ruby way to go about doing this is to override the to_s method on the model. Here we reuse the name method that's already been implemented.

igneus commented 6 years ago

Hi @Oneill38 , thanks for this PR! Travis CI reports fail, because the specs are locale-dependent. To check yourself, you can run them with all supported locales using rake spec_all_locales.

There are two ways to handle this:

Examples of both ways can be found throughout the existing specs.

Apart of this the code is perfect, but before eventually merging, I would like to clear some conceptual concerns. I will address those in #17 . It may take some time, as I would like to hear opinions of other contributors, too.

igneus commented 6 years ago

Actually, one tiny style issue: when aliasing methods, it's preferable to use alias over defining a new method calling the old one.

igneus commented 6 years ago

Hi @Oneill38 , an update: at ruby-talk I have received some very reasonable feedback and I would like to have #to_s of those three classes changed in the way suggested there.

Would you mind changing the return values accordingly?

igneus commented 6 years ago

Closing as inactive. Feel free to reopen should you wish to continue it later.