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

Aggregate doesn't seem to work with frames that cross day boudaries #248

Open
KennethNielsen opened this issue Jan 29, 2019 · 12 comments
Open

Comments

@KennethNielsen
Copy link
Contributor

Hi everyone

I just noticed the completely awesome aggregate feature in the development version (implemented by @brandomr and thanks!) and I was very thrilled because I really needed that while migrating tasks from another tool.

While migrating the tasks, I noticed that the command fails to show frames that cross day boundaries. I recognize that this is a rare usecase, but none the less I feel that it would be worth fixing.

I can help with it if necessary, but if @brandomr want's to continue the work on this command I completely understand.

@brandomr
Copy link
Contributor

@KennethNielsen that is interesting and might not be such an edge case for developers 😉. I won't have time to work on this until at least next week or the week after, so if you're able to take a whack at it, awesome!

I'm curious what occurs when this happens/what output you see?

@KennethNielsen
Copy link
Contributor Author

@brandomr the case that I had was a day that had only one frame, that ended after midnight. In that case the aggregate for that day was empty.

@KennethNielsen
Copy link
Contributor Author

KennethNielsen commented Jan 29, 2019

Hmm. As it turns out, this is a special case of report with the same start and end date not returning any output for en event that is only partially within that day. This in turn is due to Frames.span, Frames.filter and Span.__contains__ not doing that. This means that we are all the way down to the most basic objects and behaviour.

So at that point there are basically two routes:

  1. Decide that for report, this is intended behaviour... and implement aggregate without the report functionality. (This is a lot of work, duplicate effort and error prone).
  2. At some level (Frames.filter and probably report), introduce an (internal) option concerning frames that cross the boundaries. If this options is active, it will then replace the frames in question with pseudo frames with time spans that ends at the boundaries.

I tend to lean towards 2, especially considering that it can be implemented cleanly and without affecting any other functionality, but I would like some opinions before I start. What do you think @brandomr @jmaupetit?

@jmaupetit
Copy link
Contributor

I vote 2!

@loonies
Copy link
Contributor

loonies commented Mar 28, 2019

I just started using 1.7 and this is annoying. At first I thought I'm doing something wrong.

@brandomr
Copy link
Contributor

@loonies thanks for bumping this! @jmaupetit I agree on option 2. I'll take a look into fixing this next week. I'm glad to see that people are finding the feature useful (minus this bug)

@KennethNielsen
Copy link
Contributor Author

@brandomr I hope you don't mind, but since I didn't see a PR I imagined you were busy, so I tried my hand at a fix. If you are already in progress with it, then if nothing else, it can serve as something to compare with.

@brandomr
Copy link
Contributor

@KennethNielsen that is awesome! I'm sorry to have fallen off this thread--things got busy but I am so glad you picked it up. Nice!

@emarthinsen
Copy link

Any update on this one? I'll often do a long coding session that starts in the evening and ends after midnight. I use Watson to track my hours, which later get transcribed to my billing system. I just noticed large swaths of hours were missing.

@KennethNielsen
Copy link
Contributor Author

@emarthinsen the PR (#277) is pending approval of the approach, since I didn't want to keep re-basing it until I had that and I think these guys are busy. Give it a thumbs up if you want to advance it a bit.

@KennethNielsen
Copy link
Contributor Author

@emarthinsen, @brandomr @loonies the fix for this issue needs to be tested, would any of you mind. (Apologies of the get the message twice, but I was unaware if I could tag you in a PR that you didn't comment on)

@pradzins
Copy link

Thanks a lot, I was bitten by this one. Certainly, errr... very rarely! ;-)

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

No branches or pull requests

6 participants