Skip to content
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

Serialize and deserialize week start day #75

Closed
wants to merge 7 commits into from

Conversation

masqita
Copy link
Contributor

@masqita masqita commented Mar 26, 2012

My colleague forgot to serialize and deserialize the week start day (from_hash, to_hash) for weekly rules with an interval.

Tests added as well as documentation for the first day of week parameter.

@devwout
Copy link

devwout commented Mar 27, 2012

My first idea was indeed to split weekst away from WeeklyInterval in its own Validation. The problem is that they are really tightly coupled. The way dates are calculated in WeeklyInterval needs to be overwritten when you take weekst into account. Furthermore, if you take sunday as the default weekst (the way it is in ice_cube), the weekst attribute has to be present in the iCal export, since the standard defines that the default weekst is monday. When you define weekst in a separate validation, the user always has the option not to include it, and the iCal export will be wrong.

About the serialization, you are right, it should be a number. We will fix that.

@masqita
Copy link
Contributor Author

masqita commented Mar 27, 2012

The option IceCube::Rule.weekly(2, :monday) was already implemented apparently, so I just changed the docs to use the same API as the rest of the examples. Week start is now serialized as a number.

@Coffei
Copy link

Coffei commented Feb 1, 2013

I noticed, that (de)serialization with to/from yaml doesn't support week start as well. Does this fix that issue too?

@avit
Copy link
Collaborator

avit commented Feb 10, 2013

@masquita, I rebased your changes on master to do a little cleanup (whitespace, method names, etc.) and pulled this in. The API for weekly(1, :sunday) might look a little confusing vs. weekly(1).day(:sunday), but it works.

@seejohnrun if you had any other reservations about this long-waiting open ticket, do let me know but I'll close it for now.

@avit avit closed this Feb 10, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants