Skip to content

Conversation

Isira-Seneviratne
Copy link
Member

@Isira-Seneviratne Isira-Seneviratne commented Sep 18, 2023

Extract stream lengths as Duration values. Duration provides convenience methods for converting to/from different time units, e.g. seconds, hours, etc.

Note: Previously, the existing methods' types were updated to Duration, which would have required changes in existing uses. To maintain compatibility, this PR has been updated to retain the existing methods and update them to call the new methods.


  • I carefully read the contribution guidelines and agree to them.
  • I have tested the API against NewPipe.
  • I agree to create a pull request for NewPipe as soon as possible to make it compatible with the changed API.

@TobiGr TobiGr added the codequality Improvements to the codebase to improve the code quality label Sep 18, 2023
Copy link
Collaborator

@XiangRongLin XiangRongLin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know what the policy around breaking changes is anymore, otherwise it looks fine. But it should be at least mentioned that its a breaking change in the PR itself

@Isira-Seneviratne
Copy link
Member Author

I made some changes, this should no longer break existing uses of the methods.

@Isira-Seneviratne Isira-Seneviratne force-pushed the Duration branch 3 times, most recently from 3111454 to 0261e63 Compare July 12, 2024 03:28
@Stypox
Copy link
Member

Stypox commented Mar 21, 2025

I don't know if this PR is a good idea. While it's true that the correct data type would be Duration, just returning the plain old number of seconds also makes sense in my opinion. However, with Duration it would be possible to return even millisecond-precision durations, which is an advantage.

What do others think? Is it worth the effort to switch to Duration right now? Maybe it's better to do it if we ever will want to refactor the interfaces completely (in which case we wouldn't have to worry about breaking changes), but keeping track of deprecated methods and composite data types just adds complexity without clear advantages in this case. Users of the library can just use Duration.ofSeconds() if they really need a Duration.

E.g. in NewPipe we would need to change the database to properly adapt to this PR, which feels like a risky move without any advantage, which means in NewPipe we wouldn't really switch to using Duration, which makes this PR not useful (at least for NewPipe).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
codequality Improvements to the codebase to improve the code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants