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

Remove any control characters in response #54

Merged
merged 6 commits into from
Jan 30, 2022

Conversation

LevitatingBusinessMan
Copy link
Contributor

I think this should fix #43 and #7.

I couldn't find a good method to escape the control characters while retaining all other special utf-8 characters, but escaping is more prone to errors anyway as different programs interpret escape sequences differently.

The characters are removed from the lines just before being joined, encoded and send.

Currently 3 tests are failing, which seems to be caused by a mock variable not behaving the same as it's real counterpart. I am not that experienced with unittest.mock so some advice there is welcome.

I also still have to write an appropriate unit test.

Copy link
Member

@jodal jodal left a comment

Choose a reason for hiding this comment

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

Didn't look at the test failures as I'm on a phone.

mopidy_mpd/network.py Outdated Show resolved Hide resolved
mopidy_mpd/network.py Outdated Show resolved Hide resolved
@LevitatingBusinessMan
Copy link
Contributor Author

@jodal I made the character mapping a constant at the top of network.py.

I am still not sure what the best way is to deal with those test errors.

mopidy_mpd/network.py Outdated Show resolved Hide resolved
@djmattyg007
Copy link

You'll need to rebase your branch on top of master. Also I took a look at the test failure last night and am genuinely bamboozled 😲

@jodal
Copy link
Member

jodal commented Jan 25, 2022

I think we need to replace sentinel.lines with an actual iterable, like a list, for the existing tests to be compatible with this change.

@jodal jodal merged commit 722a98a into mopidy:master Jan 30, 2022
@jodal
Copy link
Member

jodal commented Jan 30, 2022

I updated the tests using sentinel.lines. Thanks for your contribution!

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.

Mopidy-MPD retains line feeds in tags in MPD response
3 participants