Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add invidious companion support #4985
base: master
Are you sure you want to change the base?
Add invidious companion support #4985
Changes from 16 commits
3dff7a7
73c84ba
1954463
c612423
2cc204a
1c9f5b0
27b24f5
409df4c
ff3305d
1aa154b
9f84612
b51770d
bb2e3b2
734e725
1f51edd
7a070fa
f710dd3
a571eea
ab72bba
1de2054
0dba767
e9c354d
f550359
bfaf72b
84b87be
a5acdde
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Codestlye nitpick
What do you think about replacing
!empty?
withpresent?
instead orunless {...}.empty?
I've seen a couple discussions in the Crystal community regarding
if !empty?
being harder to process cognitively due to an essentially double negation.Related discussions:
https://forum.crystal-lang.org/t/collections-any-vs-empty/5303
crystal-lang/crystal#13847
crystal-lang/shards#577 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's perfect for me. That was already odd for me to do
!empty?
, happy there is an alternative.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think here to redirect to a random invidious companion, rather than display an error message? Sure, if there are multiple companions, we might not hit the same that initially loaded the watch page, but it might make playback smoother.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue is that you have to do a request to a random invidious companion for knowing its "public" URL. I had ideas to include the public URL inside the config.yml. But my poor Crystal knowledge limited me due to the new URIArrayConverter.
Something like:
I want to avoid doing unnecessary requests to invidious companion.
http://127.0.0.1:8282
is the internal address that invidious uses to communicate with invidious companion. but the companion could very well be on another server, and so having a config like this can exist:10.0.0.2 is another server and 10.0.0.0/24 is an internal network faster than the internet network. example: https://www.ovhcloud.com/en/public-cloud/private-network/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something like this would be more straight forward to add
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that the simplest way to do it would be with an intermediate class, like that. Then the
URIArrayConverter
is not needed anymore, you can just use the simplerURIConverter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Invidious stream data workarounds should run when invidious companion is not set