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

Make end time match expected format (#143) #144

Merged
merged 3 commits into from
Dec 3, 2019
Merged

Make end time match expected format (#143) #144

merged 3 commits into from
Dec 3, 2019

Conversation

aywaldron
Copy link
Contributor

@aywaldron aywaldron commented Oct 23, 2019

#142 should be merged first. This branch has been rebased with changes in #142.

To test this fix works, check out this branch and follow steps 2-4 outlined in #143 .

Resolves #143

@aywaldron
Copy link
Contributor Author

Updating per @MJJoyce 's comment on #147

@MJJoyce
Copy link
Member

MJJoyce commented Nov 20, 2019

Hey @aywaldron,

I might be misunderstand the recent changes pushed here but I think it's doing something extra than was chatted about on the other ticket.

second = int(round(float(time_split[1][:-1]) / 10**9))

All of this conversion aside, shouldn't the value of this (assuming the original value is in nanoseconds) always give us a fractional number? Or said differently, we're splitting the fractional seconds off and then adding some offset to encompass the fractional time. That fractional time is always going to be at most 1 second so why not round the way mentioned in the other ticket? Specifically, split on . and then add 1 second to the end time.

I think we could really simplify both the start and end time code here to be in line with what's mentioned in the other ticket. Start time would split on . and add no offset. End times would split on . and add a 1 second offset.

Thoughts? Am I not understanding something above?

@aywaldron
Copy link
Contributor Author

aywaldron commented Nov 20, 2019

@MJJoyce it is determining whether the second should be rounded up or down rather than always rounding up. That line of code will give you either 0 or 1 depending on the value of the nanoseconds, so its more accurate than always rounding up. Maybe I'm not understanding your concern with doing this? I thought it was a concern with breaking up the time string based on index rather than splitting on . which this solution does address.

@aywaldron
Copy link
Contributor Author

Per discussion with @MJJoyce , updating to always round up for end time and round down for start time to encapsulate the correct time range.

@MJJoyce MJJoyce merged commit c160aab into master Dec 3, 2019
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.

End time in search for playback range not expected format
2 participants