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

Turn off strict parsing for pls playlist files #1923

Closed
wants to merge 1 commit into from

Conversation

djmattyg007
Copy link
Contributor

Some PLS files contain one 'Version' key for each file in the playlist. This ultimately has no impact on how mopidy parses files in the playlist, and therefore it should be as tolerant as possible of real-world playlist files.

Note that it isn't enough to simply catch configparser.DuplicateSectionError or configparser.DuplicateOptionError and keep moving, as the config object ends up in a weird half-finished state that doesn't match its state when it has finished parsing without throwing.

Consideration should also be given switching away from configparser.RawConfigParser, as the official python documentation recommends moving to configparser.ConfigParser. I'd be happy to update this PR to switch this at the same time.

Some PLS files contain one 'Version' key for each file in the playlist.
This ultimately has no impact on how mopidy parses files in the
playlist, and therefore it should be as tolerant as possible of
real-world playlist files.
@codecov
Copy link

codecov bot commented Jul 12, 2020

Codecov Report

Merging #1923 into develop will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #1923   +/-   ##
========================================
  Coverage    76.78%   76.78%           
========================================
  Files           55       55           
  Lines         4691     4691           
========================================
  Hits          3602     3602           
  Misses        1089     1089           
Impacted Files Coverage Δ
mopidy/internal/playlists.py 87.09% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c52bfba...f87fec3. Read the comment docs.

djmattyg007 added a commit to djmattyg007/mopidy-tunein that referenced this pull request Jul 12, 2020
Some PLS files contain one 'Version' key for each file in the playlist.
This ultimately has no impact on how mopidy parses files in the
playlist, and therefore it should be as tolerant as possible of
real-world playlist files.

This matches a similar change request submitted to the mopidy core:

mopidy/mopidy#1923
@kingosticks
Copy link
Member

This was a regression introduced when we ported to Python 3

https://docs.python.org/3/library/configparser.html

Changed in version 3.2: In previous versions of configparser behaviour matched strict=False.

@kingosticks kingosticks added the C-bug Category: This is a bug label Dec 5, 2020
@kingosticks
Copy link
Member

kingosticks commented Dec 6, 2020

I have to admit I had always meant to merge this as I did for Mopidy-TuneIn but I totally forgot. Thanks for the fix.

Merged in 7f1bc23

@kingosticks kingosticks closed this Dec 6, 2020
@djmattyg007
Copy link
Contributor Author

Thank you! I did email the ABC but have yet to hear back from them, so this will help a lot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants