-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Use make_client
instead of calling HTTP::Client
#4709
base: master
Are you sure you want to change the base?
Conversation
@@ -45,24 +42,22 @@ struct YoutubeConnectionPool | |||
|
|||
private def build_pool | |||
DB::Pool(HTTP::Client).new(initial_pool_size: 0, max_pool_size: capacity, max_idle_pool_size: capacity, checkout_timeout: timeout) do | |||
conn = HTTP::Client.new(url) | |||
conn.family = CONFIG.force_resolve | |||
conn = make_client(url, force_resolve = true) |
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.
It should be that, btw:
conn = make_client(url, force_resolve = true) | |
conn = make_client(url, force_resolve: true) |
I don't understand why the =
isn't flagged by the compiler as invalid syntax.
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.
So, from a quick discussion in the Crystal matrix room, doing
make_client(url, foo = true)
is the same as doing
foo = true
make_client(url, foo)
i.e it assigns the value to a local variable and then passes that variable as a positional function parameter. In this case, this would be end up being assigned to region
.
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.
Oops thanks!
I guess that also explains why this wasn't caught my the CI #4709 (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.
Yep! I've opened an issue at the Crystal bug tracker, because it seems like something that shouldn't be allowed by design: crystal-lang/crystal#14722
Using `make_client` to create `HTTP::Client`, allows for a simple way to easily add logic to all `HTTP::Client` initialized within Invidious.
Co-authored-by: ChunkyProgrammer <78101139+ChunkyProgrammer@users.noreply.github.com>
4b8c6ea
to
6e39b9b
Compare
conn.family = Socket::Family::INET if conn.family == Socket::Family::UNSPEC | ||
conn.before_request { |r| add_yt_headers(r) } if url.host == "www.youtube.com" | ||
conn | ||
next make_client(url, force_resolve: true) |
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.
This line fails Ameba's Style/RedundantNext
rule.
Should it also be disabled like Style/RedundantBegin
and Style/RedundantReturn
?
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 didn't know that next could be used like that! And yes, this will make the syntax consistent with the rest.
This PR is mostly so that we can customize the client Invidious uses for all HTTP requests. For example, to config a HTTP proxy to use.
I left
images.cr
untouched as to not conflict with #4326