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

Guard expensive debug logging with isEnabledFor #358

Merged
merged 2 commits into from
Sep 29, 2024

Conversation

bdraco
Copy link
Contributor

@bdraco bdraco commented Sep 24, 2024

Note that update_query rebuilds the whole query string every time so it would be a bit cleaner to call it once, however its probably not worth optimizing if its only going to be called when debug is on now

Note that update_query rebuilds the whole query string
every time so it would be a bit cleaner to call it once,
however its probably not worth optimizing if its only
going to be called when debug is on now
@bdraco bdraco marked this pull request as ready for review September 24, 2024 18:57
@bdraco bdraco requested a review from mib1185 as a code owner September 24, 2024 18:57
@mib1185
Copy link
Owner

mib1185 commented Sep 28, 2024

the failing tests are not related - they are caused by yarl 1.9.5 (with 1.9.4 it was working), the url does not contain the https default port 443 anymore (expected https://nas.mywebsite.me:443 got https://nas.mywebsite.me) - but yes, adding the protocols default port is redundant and not needed - will fix this first

@mib1185
Copy link
Owner

mib1185 commented Sep 28, 2024

hi @bdraco , the tests are fixed now, please rebase your branch, thx 👍

@bdraco
Copy link
Contributor Author

bdraco commented Sep 28, 2024

Thanks @mib1185 Should be all set now

Copy link
Owner

@mib1185 mib1185 left a comment

Choose a reason for hiding this comment

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

Thanks @bdraco 👍

@mib1185 mib1185 merged commit ac8c340 into mib1185:master Sep 29, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants