-
Notifications
You must be signed in to change notification settings - Fork 125
Added Week to the RollingInterval enum. #182
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
Conversation
Added a test for Week in RollingIntervalExtenstionsTests class.
Hi @camerontbelt - thanks for the PR! I'm not sure this will work - if you shut down the app, and start it up a day later, the new file will be dated only one day later than the previous one, won't it? (The format determines the stride, and not GetNextCheckpoint(), IIRC?) I'd also expect that if we're working in "calendar weeks" then we'd pick a week-start day (Sunday, or Monday), but I can't see how that happens so perhaps this is better described as rolling every "seven days"? |
@nblumhardt Re: every seven days As far as your other concern I'm not familiar enough with the code base so maybe you could help me understand. If this is a feature that will get approved I can work on it more otherwise I'm wasting both our time. |
Guys, |
I'm 100 percent agree with that. We need to specify rolling interval more accurate than just |
Hi all, Currently - not planning further work in this area; help is always welcome, but, because of limited reviewer time available, changes to this sink would need to come with a full proposal including analysis of corner cases and other impacts, with time taken to understand the existing codebase, before we could commit to working through a PR. I'm not suggesting anyone is obliged to do the spec work - but at the moment we can't really move forward on things here. |
I'm looking forward to work on this feature. Currently, I have a little time to work on it, and it might take a long time. By the way I have created the corresponding issue #213. If you have any suggestions, I appreciate your comment on the issue. |
I've added a PR that adds support for more granular intervals: |
Closing as it seems like this one has stalled on some limitations - thanks again, @camerontbelt 👍 |
Added a test for Week in RollingIntervalExtenstionsTests class.