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.

Sign up for GitHub

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

Jump to bottom

Deprecate filter #70

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Deprecate filter #70

wants to merge 8 commits into from

Conversation

swiftyy-mage
Copy link
Member

Ran make test, still getting a hell of a lot of errors in mypy. Will fix but let me know what you think so far anyway.

hbmartin
hbmartin reviewed Mar 8, 2020
View reviewed changes
Copy link
Collaborator

@hbmartin hbmartin left a comment

Choose a reason for hiding this comment

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

I'll take a look at your branch mypy results

pytube/cli.py
youtube.streams.filter(progressive=False, subtype="mp4")
.order_by("resolution")
.last()
youtube.streams.is_adaptive().subtype("mp4").order_by("resolution").last()
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks so good 🙌

Copy link
Member Author

Choose a reason for hiding this comment

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

Haha ikr!

pytube/query.py
"""
return self.fmt_streams

@deprecated("Replaced by new individual methods")
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@hbmartin
Copy link
Collaborator

hbmartin commented Mar 8, 2020

Ok... here's how I see the mypy issues:

  1. Optional["StreamQuery"] lines ... the Optional is unnecessary since these functions will always return a StreamQuery... even if it's empty
  2. Parantheses are off in the otf function, I think it should be return StreamQuery(list(filter(lambda s: s.is_otf == is_otf, self.fmt_streams)))
  3. Then there are just a couple of fixes for the res int change

@swiftyy-mage
Copy link
Member Author

Still getting a lot of errors in pytest. Will work through these now.

@swiftyy-mage
Copy link
Member Author

Got it down to 2 failures in cli.py and 9 errors, but those errors were there before. Still 60 warnings from using deprecated methods StreamQuery.filter and StreamQuery._filter, this will need addressing too. Will take another look at this with a fresh pair of eyes tomorrow, and obvs will need to still make more tests for the new methods in query.py.

@hbmartin
Copy link
Collaborator

Looks like all tests are passing! woo! Not sure we have to get to all the deprecation warnings cleaned up now... would be great to find a way to automate replacing them 😅

@swiftyy-mage
Copy link
Member Author

Ooohh nice one. I'll go through all these tests, I'm sure they will help a lot.

@swiftyy-mage
Copy link
Member Author

Just learned about pytest.mark.parameterize now, that's pretty awesome. Much simpler. Gonna keep on doing my learning on mocking now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hbmartin hbmartin hbmartin left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@swiftyy-mage @hbmartin