-
Notifications
You must be signed in to change notification settings - Fork 357
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
Update to BYSETPOS support #449
base: master
Are you sure you want to change the base?
Conversation
Hat tip to @NicolasMarlier for doing this. |
I can confirm this works as expected, this is fantastic! 👍 |
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.
I left comments to see if we can improve the code of this PR.
I'll try implementing them myself and link a diff here a bit later.
.gitignore
Outdated
@@ -8,5 +8,8 @@ | |||
/spec/reports/ | |||
/tmp/ | |||
|
|||
# rubymine | |||
.idea | |||
|
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.
Is that necessary? It looks like an accidental addition that is not part of this PR.
lib/ice_cube/time_util.rb
Outdated
@@ -198,6 +200,36 @@ def self.which_occurrence_in_month(time, wday) | |||
[nth_occurrence_of_weekday, this_weekday_in_month_count] | |||
end | |||
|
|||
# Use Activesupport CoreExt functions to manipulate time | |||
def self.start_of_month time | |||
time.beginning_of_month |
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.
Why not reuse the same name?
lib/ice_cube/time_util.rb
Outdated
|
||
# Use Activesupport CoreExt functions to manipulate time | ||
def self.start_of_year time | ||
time.beginning_of_year |
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.
Why not reuse the same name?
lib/ice_cube/time_util.rb
Outdated
|
||
# Use Activesupport CoreExt functions to manipulate time | ||
def self.previous_month time | ||
time - 1.month |
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.
I think time.last_month
exists?
lib/ice_cube/time_util.rb
Outdated
|
||
# Use Activesupport CoreExt functions to manipulate time | ||
def self.previous_year time | ||
time - 1.year |
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.
I think time.last_year
exists?
return by_set_pos([by_set_pos]) if by_set_pos.is_a?(Integer) | ||
|
||
unless by_set_pos.nil? || by_set_pos.is_a?(Array) | ||
raise ArgumentError, "Expecting Array or nil value for count, got #{by_set_pos.inspect}" |
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.
This validation seems unnecessary as well. by_set_pos
is always an Array
.
end | ||
by_set_pos.flatten! | ||
by_set_pos.each do |set_pos| | ||
unless (set_pos >= -366 && set_pos <= -1) || |
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.
How about (-366..366).include(set_post) && set_pos != 0
?
end | ||
|
||
@by_set_pos = by_set_pos | ||
replace_validations_for(:by_set_pos, by_set_pos && [Validation.new(by_set_pos, self)]) |
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.
If I understand correctly, by_set_pos
cannot be nil
or false
here (it's an Array
), in which case by_set_pos && [Validation.new(by_set_pos, self)]
is equivalent to its right-side operand, [Validation.new(by_set_pos, self)]
.
Hello @davidstosik, @NicolasMarlier provided this original PR. I simply updated it to work with modern versions of Ruby. |
|
||
|
||
new_schedule = IceCube::Schedule.new(TimeUtil.previous_month(step_time)) do |s| | ||
s.add_recurrence_rule IceCube::Rule.from_hash(rule.to_hash.reject{|k, v| [:by_set_pos, :count, :until].include? k}) |
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.
With ActiveSupport, this reject
construct simplifies to except(:by_set_pos, :count, :util)
|
||
|
||
new_schedule = IceCube::Schedule.new(TimeUtil.previous_year(step_time)) do |s| | ||
s.add_recurrence_rule IceCube::Rule.from_hash(rule.to_hash.reject{|k, v| [:by_set_pos, :count, :until].include? k}) |
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.
same here, use Hash#except
spec/examples/by_set_pos_spec.rb
Outdated
schedule = IceCube::Schedule.from_ical "RRULE:FREQ=MONTHLY;COUNT=4;BYDAY=WE;BYSETPOS=4" | ||
schedule.start_time = Time.new(2015, 5, 28, 12, 0, 0) | ||
expect(schedule.occurrences_between(Time.new(2015, 01, 01), Time.new(2017, 01, 01))).to eq([ | ||
Time.new(2015,6,24,12,0,0), |
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.
I'd say just breaking .to
onto a newline is good enough
In August 2016, @NicolasMarlier added BYSETPOS support (ice-cube-ruby#349). Then, in July 2018, @nehresma added a few small changes to run in modern Ruby and a more modern rspec (ice-cube-ruby#449). Then, in January 2019, @davidstosik and @k3rni suggested changes to reduce complexity. This incorporates all the above into a single diff.
@nehresma what's the status of this PR? I'd very much like to use the |
@dimerman It has been merged but I do not believe a new version of the gem has been released. I've pointed my Gemfile to the latest git ref on master for the time being. Hopefully someone will have a few minutes to cut a new gem release at some point so we wont have to point to specific refs. |
@dimerman Err, no -- I'm an idiot. I just realized it has NOT been merged. I checked my Gemfile and I'm pointing to the ref with BYSETPOS support out on my fork -- not master on seejohnrun/ice_cube. Sorry for the confusion. I should have double checked before trusting my memory. |
@nehresma no worries - I've been pointing my gemfile to your branch/repo. thanks for that! However, I found the following case is not working right:
when I create a |
What's still holding this back — the PR feedback seems pretty minor? If that gets cleaned up can we get this merged? |
@seejohnrun It seems like several people need this and are using forks to get around it being missing. I'd be more than happy to help clean this up to avoid using a fork myself if it is otherwise merge-able. |
|
||
|
||
def build_s(builder) | ||
builder.piece(:by_set_pos) << by_set_pos |
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.
Hello @nehresma
Looks like this should be improved:
> IceCube::Rule.monthly(2).day(1,2,3,4,5).by_set_pos(2).to_s
"Every 2 months on Weekdays [2]"
@nehresma / @mattbooks / @ovinix - If you can get this PR cleaned up and resolve the feedback and conflicts with |
Hello, looks like the gem IS already supporting what BYTSETPOS does but in different ical format. Example: If we want recurring for every 2 months 3rd Wednesday, ical format w.r.t the BYSETPOS is FREQ=MONTHLY;INTERVAL=2;BYDAY=WE;BYSETPOS=3 which is same as FREQ=MONTHLY;INTERVAL=2;BYDAY=3WE, the latter one is already supported by the gem, so not sure why we still wants this PR. |
I don’t use the gem anymore, but you might want to consider that some people use the gem to parse strings produced by another piece of software they don’t have control on, and this would require the whole specification to be supported. 😉 |
@avit Anything we can do to help get this merged? |
@davidstosik what gem do you use instead? |
Nothing. 😬 |
A few small updates to Nicolas Marlier's BYSETPOS support added in PR ice-cube-ruby#349
rebased against master -- its been 4 years
It's been over 4 years since I created this PR and over 7 years since Nicolas Marlier's initial commit with this support. I have rebased it against current master and I've applied changes per the feedback received above. If someone has time to review and merge, that would be most appreciated. BYSETPOS is part of RFC 5545 and it would be great if this gem could finally get support merged in rather than just the "noop" that it currently is (https://github.com/ice-cube-ruby/ice_cube/blob/master/lib/ice_cube/parsers/ical_parser.rb#L77-L78). |
@jkehres I've fixed the interval use for month and year frequencies with bysetpos. I ran out of time for adding support for the other frequencies yet. I'll add support for those after Christmas. |
I've pushed bysetpos support for weekly frequencies too. I don't think bysetpos makes sense with frequencies of daily, hourly, minutely, or secondly. RFC 5545 makes no mention of them, and indeed all of it's examples are with monthly and weekly frequencies. If someone can think of an example where using bysetpos with one of these other frequencies makes sense, let me know. |
@@ -78,7 +78,7 @@ module IceCube | |||
end | |||
|
|||
it "should be able to make a round-trip to YAML with .day_of_year" do | |||
schedule1 = Schedule.new(Time.now) | |||
schedule1 = Schedule.new(Time.zone.now) |
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.
Time.zone
is another active support extension. The stock Time
class in Ruby has no zone
method.
I believe BYSETPOS support is still needed for daily, hourly, and minutely frequencies to be compliant with the RFC, which I think should be the goal of this PR. As I mentioned in my comment here, you can see BYSETPOS having an effect for these other frequencies using rrule.js - another library that implements RFC 5545 (itself a port of the python-dateutil library). If ice_cube and these other libraries all properly implement the spec, they should all give the same answer for the same RRULE. We could use that fact to aid in testing this PR to ensure we have a compliant implementation of the RFC. We could us one of these other libraries to generate a more exhaustive set of test inputs and outputs for RRULEs that use BYSETPOS and, thus, increase the number of tests for this new code along with our confidence in it. Secondly, even though the RFC gives no examples for these other frequencies with BYSETPOS, I think you can derive that they are necessary from the details in the spec. From the description of BYSETPOS (emphasis mine):
My understanding of the spec is that "interval" in this context means one unit of the frequency regardless of the INTERVAL rule part so one year for yearly, one month for monthly, etc. If there are multiple recurrences instances within one interval (e.g. year, month, etc.), then BYSETPOS has an effect and selects the nth one. Normally, there is only a single recurrence instance within an interval. The way you get more than one recurrence instance in an interval is by adding another BYxxx rule part. Again, from the spec (emphasis mine):
Furthermore, this table from the spec shows how each BYxxx rule part interacts with a given frequency and whether it expands or limits the number of recurrences instances:
For secondly, you can see that no BYxxx rule part will expand the set of recurrence instances so BYSETPOS won't have an effect. However, for minutely, hourly, and daily frequencies you can see that some BYxxx rule parts will expand the set of recurrences, which would allow BYSETPOS to select the nth one in an interval and thus alter the final set of recurrence instances. |
describe WeeklyRule, 'BYSETPOS' do | ||
it 'should behave correctly' do | ||
# Weekly on Monday, Wednesday, and Friday with the week starting on Wednesday, the last day of the set | ||
schedule = IceCube::Schedule.from_ical("RRULE:FREQ=WEEKLY;COUNT=4;WKST=WE;BYDAY=MO,WE,FR;BYSETPOS=-1") |
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.
For each of the frequencies, it would be nice to have the following test cases:
- With a BYxxx that expands the set of recurrence instances...
- BYSETPOS with a positive value
- BYSETPOS with a negative value
- BYSETPOS with multiple positive values
- BYSETPOS with multiple negative values
- BYSETPOS with multiple values both positive and negative
- BYSETPOS with multiple repeated values - spec does not define behaviour; rrule.js does some weird things and outputs the same occurrence multiple times (bug or maybe just undefined behaviour?)
- BYSETPOS with a value that matches in some intervals but not others (e.g. selecting the 5th Monday of every month since some months will have only 4 Mondays)
- BYSETPOS with a positive value beyond than the number of recurrence instances in any interval (e.g. the 10th Monday of every month); rrule.js returns an empty set of occurrences
- BYSETPOS with a negative value bigger than the number of recurrence instances in any set (e.g. the -10th Monday of every month); rrule.js returns a non-empty set of occurrences, which is weird (bug or maybe just undefined behaviour?)
- BYSETPOS with each BYxxx that expands the set of recurrence instances for the current frequency
- BYSETPOS with BYxxx that limits the set of recurrence instances instead of expanding it
- BYSETPOS with no BYxxx - spec says BYSETPOS "MUST only be used in conjunction with another BYxxx rule part" so not sure if this should be an exception or an empty set of occurrences returned; rrule.js does the latter
- BYSETPOS with multiple BYxxx rules parts that expand the recurrence instances
- BYSETPOS with multiple BYxxx rules parts that both expand and limit the recurrence instances
- BYSETPOS with a value outside the allowed range (covered in the
from_ical
tests)
@@ -75,7 +75,7 @@ def self.rule_from_ical(ical) | |||
when "BYYEARDAY" | |||
validations[:day_of_year] = value.split(",").map(&:to_i) | |||
when "BYSETPOS" | |||
# noop | |||
params[:validations][:by_set_pos] = value.split(',').collect(&:to_i) |
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.
Perhaps use the style validations[:by_set_pos]
to match everything else in this case statement.
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.
Also, existing code uses double quotes instead of single quotes for string in split
and collect
instead of map
. Picky but just for consistency.
require 'date' | ||
require 'time' |
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.
Changing the type of quotes seems unnecessary. Double quotes are consistent with the existing style.
], step_time | ||
) | ||
new_schedule = IceCube::Schedule.new(start_of_week_adjusted) do |s| | ||
s.add_recurrence_rule(IceCube::Rule.from_hash(rule.to_hash.except(:by_set_pos, :count, :until))) |
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.
except
is another active support method:
|
||
# Needs to start on the first day of the month | ||
new_schedule = IceCube::Schedule.new(IceCube::TimeUtil.build_in_zone([start_of_month.year, start_of_month.month, start_of_month.day, step_time.hour, step_time.min, step_time.sec], start_of_month)) do |s| | ||
s.add_recurrence_rule(IceCube::Rule.from_hash(rule.to_hash.except(:by_set_pos, :count, :until))) |
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.
Ditto my comment about except
.
|
||
# Needs to start on the first day of the year | ||
new_schedule = IceCube::Schedule.new(IceCube::TimeUtil.build_in_zone([start_of_year.year, start_of_year.month, start_of_year.day, step_time.hour, step_time.min, step_time.sec], start_of_year)) do |s| | ||
s.add_recurrence_rule(IceCube::Rule.from_hash(rule.to_hash.except(:by_set_pos, :count, :until))) |
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.
Ditto my comment about except
.
Using BYSECOND with MONTHLY does not work. I tried this: ical = "RRULE:FREQ=MONTHLY;COUNT=4;BYSECOND=1,2,3,4;BYSETPOS=2"
schedule = IceCube::Schedule.from_ical(ical)
schedule.start_time = Time.new(2015, 5, 28, 12, 0, 0)
schedule.occurrences_between(Time.new(2015, 01, 01), Time.new(2017, 01, 01)) and got an empty array. I expected to get:
I suspect there is an assumption in the code that you will only use BYDAY with MONTHLY and not other BYxxx rule parts. |
First I want to thank @nehresma @jkehres and others for all the work on this.
The current code already doesn't implement the RFC, and it fails to do so silently (just spent over an hour finding the this as the source of a bug). I think partial RFC compliance is better than none. And the users who use daily/hourly/minutely frequencies would continue to see the current unimplemented behavior. I know I wouldn't be alone in appreciating getting this work merged. |
Thanks for the review @jkehres. Can we get the tests running/passing? |
Thank you so much for your work here. I'd love if we can get this merged. Apple Calendar feeds use |
@jorgemanrubia - Since nothing has moved on this PR for a long time ... it might be easiest if you can fork it, get the tests passing and then we can move forwards? I have struggled for time recently otherwise would have done this myself. |
The RRULE parser does not support BYPOS, so instead of 'every 2nd tuesday of the month' you get 'every tuesday on the month'. Pulls in the fork [this PR](ice-cube-ruby/ice_cube#449) comes from for the fix, and adds a test to assert it.
Monthly/Yearly would be more than enough for me. |
In August 2016, Nicolas Marlier added BYSETPOS support (#349). This PR updates that PR with a few small changes to run in modern Ruby (use Integer instead of Fixnum) and a more modern rspec.