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 properties in ITag available in public-facing API #22

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Jeehut
Copy link

@Jeehut Jeehut commented Sep 12, 2023

Fixes #10.

@waliid
Copy link
Contributor

waliid commented Oct 1, 2023

What do you think about keeping things private and adding a new API like:

func streams(withLessOrEqualResolution resolution: Int) -> [Stream] {
    filter { $0.itag.videoResolution ?? 0 <= resolution }
}

@Jeehut
Copy link
Author

Jeehut commented Oct 1, 2023

Not very flexible IMO. People might have much more complex logic than what I've done here.

@alexeichhorn
Copy link
Owner

I guess we can make these properties public since they are inside the itag property anyway. But please revert the innertube client type back to what it was. Putting web there just breaks other things.

@alexeichhorn
Copy link
Owner

What do you think about keeping things private and adding a new API like:

func streams(withLessOrEqualResolution resolution: Int) -> [Stream] {
    filter { $0.itag.videoResolution ?? 0 <= resolution }
}

And I like this kind of filter functions. Could still be helpful, since fiddeling inside itags is not that straight forward.

@Jeehut
Copy link
Author

Jeehut commented Oct 6, 2023

@alexeichhorn I need the .web for my use case. But I did that change in a separate commit, so If you just want the public API part only, simply cherry-pick d6fcd96 and close this PR. Or simply do the changes on your main branch, it's simply adding the public keyword at a few places. Should be easy to copy.

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.

'videoResolution' is inaccessible due to 'internal' protection level
3 participants