-
Notifications
You must be signed in to change notification settings - Fork 21
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 .each method to calendar #25
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, thank you very much for your contribution to calendarium-romanum! In the contributed code I've found two issues which need to be fixed before merging.
spec/calendar_spec.rb
Outdated
@@ -33,6 +33,14 @@ | |||
end | |||
end | |||
|
|||
describe '.each' do | |||
it 'yields calendar day instances' do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add another test checking that each
actually yields. This one only checks what it yields if it yields.
lib/calendarium-romanum/calendar.rb
Outdated
@@ -126,6 +126,12 @@ def day(*args, vespers: false) | |||
) | |||
end | |||
|
|||
def each | |||
(temporale.start_date..temporale.end_date) | |||
.map { |date| day(date) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both time and memory are being wasted here:
map
iterates over the date range, retrieves calendar data and collects them in a new arrayeach
iterates over this array and yields it's elements
Both (retrieve calendar data, yield them) should be done in a single each
. This way you save one iteration as well as memory necessary for the temporary array.
Hi @igneus - I really appreciate your feedback. Thanks for your patience. This is my first time using some of rspec's yield-matchers! Please review and let me know if you have any further feedback. |
Thanks, now everything is OK. Merging. |
closes #14
@igneus - please let me know if you'd like any modifications to anything! it seems simple, but took me some time to orient myself to the repo.