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

Publicly expose Calendar::addedHolidays and Calendar::removeHolidays #629

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@igitur
Copy link
Contributor

commented May 9, 2019

For calendars, I've found it useful to have the list of added and remove public holidays exposed.

This PR adds the necessary methods to expose them. Not sure if this is the correct implementation and whether the std::set<Date> should by returned by value or by reference. This PR currently returns it by value, which probably means the user cannot alter the underlying items in the set (not sure). Please guide me as you think this should be implemented, if at all.

@igitur igitur force-pushed the igitur:expose-holiday-modifications branch from 6c8772b to 7695b25 May 9, 2019

@lballabio lballabio added this to the 1.16 release milestone May 14, 2019

@igitur

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2019

@lballabio Am I right in saying that addedHolidays() will return a std::set<Date> and therefore with the returned value, the user will still be able to modify the collection? Ideally we would want to return just some kind of read-only enumerable or iterator. Something like the C++ equivalent of C#'s IEnumerable<DateTime>. I'm not sure what that would be.

@lballabio

This comment has been minimized.

Copy link
Owner

commented May 20, 2019

No, you're returning the set by copy. The user will be able to modify the copy, but not the set inside the calendar.

@igitur

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2019

I was hoping to return some type that doesn't even have an add() method, as to avoid confusion. Or are you OK with it as it is?

@lballabio

This comment has been minimized.

Copy link
Owner

commented May 20, 2019

I'm ok.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.