Update to BYSETPOS support #449
Conversation
|
Hat tip to @NicolasMarlier for doing this. |
|
I can confirm this works as expected, this is fantastic! 👍 |
davidstosik
left a comment
davidstosik
left a comment
There was a problem hiding this comment.
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
|
|
||
| # rubymine | ||
| .idea | ||
|
|
There was a problem hiding this comment.
Is that necessary? It looks like an accidental addition that is not part of this PR.
lib/ice_cube/time_util.rb
Outdated
|
|
||
| # Use Activesupport CoreExt functions to manipulate time | ||
| def self.start_of_month time | ||
| time.beginning_of_month |
lib/ice_cube/time_util.rb
Outdated
|
|
||
| # Use Activesupport CoreExt functions to manipulate time | ||
| def self.start_of_year time | ||
| time.beginning_of_year |
lib/ice_cube/time_util.rb
Outdated
|
|
||
| # Use Activesupport CoreExt functions to manipulate time | ||
| def self.previous_month time | ||
| time - 1.month |
lib/ice_cube/time_util.rb
Outdated
|
|
||
| # Use Activesupport CoreExt functions to manipulate time | ||
| def self.previous_year time | ||
| time - 1.year |
| 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.
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.
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.
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.
With ActiveSupport, this reject construct simplifies to except(:by_set_pos, :count, :util)
There was a problem hiding this comment.
Yes but ActiveSupport isn't a runtime requirement of this gem.
|
|
||
|
|
||
| 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}) |
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.
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.
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. 😬 |
|
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). |
|
I'll take a look. Am going to try to dedicate sometime this holiday season to get things tidied up and in better shape. |
|
Trying this out for monthly frequencies with an interval > 1 seems to get stuck in an infinite loop. I used the monthly test case from this PR and ran the following code, which defaults to an interval of 1, and everything works fine: 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)
schedule.next_occurrence(Time.new(2015, 01, 01))
=> 2015-06-24 12:00:00 -0400However, if I add an interval value > 1 then schedule = IceCube::Schedule.from_ical "RRULE:FREQ=MONTHLY;COUNT=4;BYDAY=WE;BYSETPOS=4;INTERVAL=2"
schedule.start_time = Time.new(2015, 5, 28, 12, 0, 0)
schedule.next_occurrence(Time.new(2015, 01, 01))
# method hangsIf I add a The same thing happens for yearly frequencies with an interval > 1. |
|
I don't think this PR is sufficient to fully support BYSETPOS. It only deals with monthly and yearly frequencies. However, playing around in the rrule.js demo site, BYSETPOS can have an effect on shorter frequencies too. For example these rule pairs each return different results with and without BYSETPOS: |
A few small updates to Nicolas Marlier's BYSETPOS support added in PR ice-cube-ruby#349
rebased against master -- its been 4 years
This was a holdover from the original PR back in 2016. TimeUtil has since been refactored to not need this, but the require was inadvertently left.
- weekly: positive/multi/mixed positions, BYHOUR expansion - monthly: negative positions, BYMONTHDAY, BYMINUTE - yearly: multiple positive/negative positions
This covers repeated values, out-of-range, and UNTIL cases
|
Thanks everyone for the patience on this long‑running BYSETPOS PR. It has been 9 years since @NicolasMarlier first began work on this, and over 7 since I added to it. I’ve rebased on current master, worked through every prior comment, and expanded the implementation/tests to full RFC 5545 behavior for all BYxxx rule parts ice_cube supports. In short: BYSETPOS now works across all frequencies that ice_cube implements (SECONDLY -> YEARLY), applies per‑interval after all BYxxx parts (per RFC ordering), preserves DTSTART anchors, and is fully covered by tests. I also fixed several long‑standing edge cases identified in review comments (interval > 1 hangs, BYSECOND+MONTHLY empty sets, multi‑rule COUNT undercounting, etc.). What’s addressed, mapped to prior feedback:
RFC 5545 alignment (for supported BYxxx parts)
Intentional divergence
Summary of changes from master
Test coverage highlights (new/expanded)
I believe this brings BYSETPOS to full RFC‑correct behavior for all BYxxx parts ice_cube supports, with comprehensive coverage. Happy to answer questions or tweak any remaining concerns. I would love to get this merged into master. :) |
13 participants
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.