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

Fix #937: Local playlists refactoring. #1000

Merged
merged 1 commit into from Feb 25, 2015

Conversation

3 participants
@tkem
Member

tkem commented Feb 25, 2015

Rebase of #995.

path = local_playlist_uri_to_path(uri, self._playlists_dir)
if os.path.exists(path):
os.remove(path)

This comment has been minimized.

@adamcik

adamcik Feb 25, 2015

Member

try/except around this just to be on the safe side and not crash the actor?

This comment has been minimized.

@tkem

tkem Feb 25, 2015

Member

I'm a little undecided about this, giving your instructions about OSError and handling it with logging ;-)
The old code read:

       file_path = self._m3u_uri_to_path(uri)
       if os.path.exists(file_path):
           os.remove(file_path)

else "nada", so I tried to keep close to that, but do some logging in the else branch (I spent a whole evening trying to figure out what was actually wrong with local playlists, and this would have definitely helped)-

if index >= 0 and uri != playlist.uri:
path = local_playlist_uri_to_path(uri, self._playlists_dir)
if os.path.exists(path):
os.remove(path)

This comment has been minimized.

@adamcik

adamcik Feb 25, 2015

Member

try/except?

This comment has been minimized.

@tkem

tkem Feb 25, 2015

Member

In this case, I'd rather escalate the error to the client, because something must have been gone terribly wrong. One use case I can think of is lack of access rights: We don't want to remove the playlist from self._playlists if we can't delete the file, I guess.

This comment has been minimized.

@adamcik

adamcik Feb 25, 2015

Member

What if we can't read it, for practical purposes it wouldn't exist after a reboot. And come think of it we would also error out on writing the file, only open is currently handled correctly.

@adamcik

This comment has been minimized.

Member

adamcik commented Feb 25, 2015

Didn't find anything beyond making the os.remove safe from a rather unlikely race condition.

@adamcik

This comment has been minimized.

Member

adamcik commented Feb 25, 2015

So I'm happy to get this in as it stands now and open a new issue for the OSError and IOError cases I mentioned for write and delete. Or you can just get these fixed as part of this PR.

What would you prefer?

@tkem

This comment has been minimized.

Member

tkem commented Feb 25, 2015

Well, after re-thinking this in general (after becoming a member of the team :-), I guess I prefer more but smaller PRs. So I'd be happy if we get done with this as-is, and concentrate on the other issues seperately.

@tkem

This comment has been minimized.

Member

tkem commented Feb 25, 2015

And, hey, happy #1000 to everybody who made this happen ;-)

@jodal

This comment has been minimized.

Member

jodal commented Feb 25, 2015

Yay! :-)

adamcik added a commit that referenced this pull request Feb 25, 2015

Merge pull request #1000 from tkem/fix/937
Fix #937: Local playlists refactoring.

@adamcik adamcik merged commit 8433476 into mopidy:develop Feb 25, 2015

2 checks passed

Scrutinizer 3 updated code elements
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@tkem tkem deleted the tkem:fix/937 branch Feb 25, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment