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

Allow usage of arrays in addition to variadic arguments in Interval and Plan functions #25

Merged
merged 6 commits into from Nov 27, 2018

Conversation

WilsonGramer
Copy link
Contributor

Although it is cleaner to use a variadic function when creating schedules (ex. via Plan.of(_ dates: Date...)), sometimes it is necessary to pass an array instead of a list of parameters, like when dealing with data obtained at runtime. This PR adds that ability. For example, the Plan.of(_:) function can now take an array of Dates like so:

let dates = [Date(), Date(), Date(), Date(), Date()]
Plan.of(dates).do { ... }

The existing API has not changed whatsoever, the only difference being that the function implementations are now located in the array-variant of the function, and the existing variadic-variant (whose parameters are interpreted as an array in the function body) now simply passes its parameters into the array-variant.

@codecov-io
Copy link

codecov-io commented Nov 25, 2018

Codecov Report

Merging #25 into master will increase coverage by 0.17%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #25      +/-   ##
==========================================
+ Coverage   90.71%   90.88%   +0.17%     
==========================================
  Files          16       16              
  Lines         840      834       -6     
==========================================
- Hits          762      758       -4     
+ Misses         78       76       -2

@luoxiu
Copy link
Owner

luoxiu commented Nov 26, 2018

Hi, @Wilsonator5000 , thank you for your contribution to the project!
Yes, sometimes we do need to use an array as a parameter! I saw your changes, it seems you haven't considered the case of an empty array, which could cause out of bounds errors.

@WilsonGramer
Copy link
Contributor Author

WilsonGramer commented Nov 26, 2018

Hi @jianstm,

Yep, I forgot to consider that. Should I implement it where the functions return nil if an empty array is given, throw an error or use a precondition (crash if empty) so the existing API doesn’t change?

Thanks,
Wilson

@luoxiu
Copy link
Owner

luoxiu commented Nov 26, 2018

Just return a never plan.

Needed for unit tests to compare different Plan instances. The == check is based on Plan.sequence.
The == check was only reliable on never Plans and failed for Plans with dates or intervals, so it was removed and replaced with the reliable, single-purpose isNever() check.
@WilsonGramer
Copy link
Contributor Author

OK, I added the checks and some corresponding unit tests. I originally made Plan equatable so I could test for never Plans in the unit tests, but it proved to be unreliable. Instead I created an isNever function that only works for never Plans. Although an Equatable check would be useful, I think it's out of the scope of this PR.

@luoxiu luoxiu merged commit 3b58174 into luoxiu:master Nov 27, 2018
@luoxiu
Copy link
Owner

luoxiu commented Nov 27, 2018

@Wilsonator5000 Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants