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

Bugfix: HTTPSocks.pm - Binary OR used by mistake - Logical OR needed #843

Merged
merged 1 commit into from Dec 12, 2022

Conversation

mw9
Copy link
Contributor

@mw9 mw9 commented Dec 11, 2022

I fell across these two items by chance recently, so offer them up while fresh in my mind.

  • Slim::Networking::Async::Socket::HTTPSocks

The binary OR operator | has been inadvertently used in place of the intended logical OR operator ||.

This was typo in PR #562 - flac & socks fixes. Refer commit: 907a6a

The error/typo is quite obvious when you examine the commit, but @philippe44 (author) might like to confirm.

It could result in some 'hard to fathom out' socks connection failures if left as is.

  • Slim::Networking::Async::HTTP

Remove redundant comment. The code to which this referred was refactored away some years ago.

HTTPSocks.pm

The binary OR operator (single pipe character) has been inadvertently
used in place of the intended logical OR operator (double pipe char). 

This was typo in change LMS-Community#562 - flac & socks fixes. Refer:
LMS-Community@907a6a


Async/HTTP.pm

Remove redundant comment. The code to which this referred was refactored
away some years ago.
@mherger mherger changed the base branch from public/8.4 to public/8.3 December 12, 2022 05:56
@mherger mherger merged commit 9d57bcf into LMS-Community:public/8.3 Dec 12, 2022
@mherger
Copy link
Contributor

mherger commented Dec 12, 2022

Good catch! Should have been mine 😀.

@philippe44
Copy link
Contributor

Indeed (not that it should have been yours @mherger, but it's a good catch that did not bite because it was a numeric field with 0 as a default)

@mw9 mw9 deleted the feature/bugfix branch December 12, 2022 14:35
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.

None yet

3 participants