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

Fix overridable methods in MediaPipeline #6365

Closed
wRAR opened this issue May 15, 2024 · 1 comment · Fixed by #6368
Closed

Fix overridable methods in MediaPipeline #6365

wRAR opened this issue May 15, 2024 · 1 comment · Fixed by #6368
Labels

Comments

@wRAR
Copy link
Member

wRAR commented May 15, 2024

MediaPipeline defines several empty or almost empty "overridable" methods, which return things inconsistent with their overrides. I propose making all of them raise NotImplementedError. Alternatively MediaPipeline should just be made an abstract class and all those methods made abstract methods, but I have no idea if that will break anything (e.g. do all children always override all of those methods?).

Another problem is existing tests, that test specifically that e.g. MediaPipeline.media_downloaded() returns a response, which makes no sense to me (normally media_downloaded() returns a file info dict), so all those need to be changed or removed.

And another problem, indirectly related to this, is that this interface is very poorly documented, most of these functions are not mentioned in the docs at all, so it's not always clear what should they take and return (and the code uses many of them as callbacks in long callback chains so it's not clear even from the code).

@wRAR wRAR added the bug label May 15, 2024
@wRAR
Copy link
Member Author

wRAR commented May 15, 2024

A tentative proposal:

  • Replace all bodies with raise NotImplementedError().
  • Remove all tests that test calling these methods, they are not useful for anything even now.
  • Make sure nothing else broke.
  • Make the methods abstract, make sure nothing broke.
  • Ideally get some 3rd-party pipelines and check that they didn't break either (or just check that they override all of these methods, directly or via FilesPipeline/ImagesPipeline, in which case they can't break?)?

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

Successfully merging a pull request may close this issue.

1 participant