-
-
Notifications
You must be signed in to change notification settings - Fork 516
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 getters for rrules, exrules, rdates, exdates #347
Conversation
Codecov Report
@@ Coverage Diff @@
## master #347 +/- ##
==========================================
+ Coverage 89.74% 90.06% +0.31%
==========================================
Files 27 28 +1
Lines 1902 1962 +60
Branches 578 583 +5
==========================================
+ Hits 1707 1767 +60
Misses 195 195
Continue to review full report at Codecov.
|
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.
So, these properties are deliberately private, because they're not meant to be mutable from the outside. I'm concerned if we expose these getters, consumers will start mutating them, and then expecting those mutations to take effect. I'm not interested in supporting that at this time, and in fact, want to get away from mutability altogether. If there were a way to return copies of the properties, such that changing the copies did not change the original value, then we could accept that.
@davidgoli sure thing -- I agree with your logic, our use case is not to modify them at all -- we just need to be able to generate RRules from an RRuleset for an RRuleset visual editor. I've modified the PR accordingly. |
set.rrule(rrule); | ||
|
||
expect(set.rrules().map(e => e.toString())).eql([rrule.toString()]); | ||
}); |
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.
one small style nitpick: can we make sure there are blank lines in between it
blocks? thanks!
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.
Done
@davidgoli could you create a release for this? Would be greatly appreciated -- thanks! |
@epicfaace Published v2.6.2. Cheers! |
Thanks for contributing to
rrule
!To submit a pull request, please verify that you have done the following:
master
commitaddressing
yarn build
to rebuild thedist/
files