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 #1218 #1219

Merged
merged 5 commits into from Jul 22, 2015

Conversation

3 participants
@fatg3erman
Contributor

fatg3erman commented Jul 12, 2015

Output the last_modified timestamp from mopidy's track model to mpd clients
in the same format as mpd uses - yyyy-mm-ddTHH:MM:SS

Outputs nothing for Last-Modified if last_modified is None

fatg3erman added some commits Jul 12, 2015

Fix #1218
Output the last_modified timestamp from mopidy's track model to mpd clients
in the same format as mpd uses - yyyy-mm-ddTHH:MM:SS

Outputs nothing for Last-Modified if last_modified is None
Fix #1218
Output a track's Last-Modified stamp in ISO 8601 format, as MPD does.

Output nothing if track has no last-modified stamp.

The test has to use datetime to work out what the output will look like,
because it is local-time zone dependant.
Fix #1218 - Last-Modified timestamp for tracks
Don't know why the Travis build failed, it's passing for me
@fatg3erman

This comment has been minimized.

Contributor

fatg3erman commented Jul 13, 2015

I don't understand why this is failing. The failure is occurring in code I haven't changed, and it is all passing when I run tox locally.

@adamcik

This comment has been minimized.

Member

adamcik commented Jul 20, 2015

87bf261 fixed this, turns out we've had this unnoticed typo for a few years...

@jodal

This comment has been minimized.

Member

jodal commented Jul 20, 2015

Does MPD include a trailing 'Z' or similar to signal the time zone? Since we are converting from Unix epoch, we are time zone aware. It would be nice if that was reflected in the timestamp to the MPD client.

@fatg3erman

This comment has been minimized.

Contributor

fatg3erman commented Jul 20, 2015

Yes it does, at least for me MPD returns a trailing Z. Now I need to figure out how to get python to do the same. Any hints..? :)

self.assertIn(('Track', '7/13'), result)
self.assertIn(('Date', '1977-01-01'), result)
self.assertIn(('Disc', 1), result)
self.assertIn(('Last-Modified', datestring), result)

This comment has been minimized.

@jodal

jodal Jul 20, 2015

Member

I think datestring should be written out here, instead of calculating it with the same logic as used in the implementation.

@@ -86,6 +87,11 @@ def track_to_mpd_format(track, position=None, stream_title=None):
if track.disc_no:
result.append(('Disc', track.disc_no))
if track.last_modified is not None:
datestring = datetime.datetime.fromtimestamp(

This comment has been minimized.

@jodal

jodal Jul 20, 2015

Member

Use datetime.datetime.utcfromtimestamp instead, and this will no longer depend on the local time zone.

Then you can safely just append "Z" to the returned string yourself, since the returned timestamp will always be in the UTC time zone.

@jodal jodal added this to the v1.1 - Robust core milestone Jul 20, 2015

@jodal jodal self-assigned this Jul 20, 2015

Fix #1218
Output the last_modified timestamp from mopidy's track model to mpd clients
in the same format as mpd uses - yyyy-mm-ddTHH:MM:SS

Outputs nothing for Last-Modified if last_modified is None or zero

This commit uses UTC time, adds a 'Z' to end, and updates the test accordingly
@fatg3erman

This comment has been minimized.

Contributor

fatg3erman commented Jul 21, 2015

OK I've updates this to use UTC time, and changed the test to use a predefined string.
Also changed it from 'if lastmodified is not None' to just 'if lastmodified', as I think I recall once seeing something having a zero for lastmodified, and that's not helpful or useful to a client.

@jodal

This comment has been minimized.

Member

jodal commented Jul 21, 2015

Haven't looked at the code, but the 0 case sounds like something we want a
test for.

On Tue, Jul 21, 2015, 19:25 fatg3erman notifications@github.com wrote:

OK I've updates this to use UTC time, and changed the test to use a
predefined string.
Also changed it from 'if lastmodified is not None' to just 'if
lastmodified', as I think I recall once seeing something having a zero for
lastmodified, and that's not helpful or useful to a client.


Reply to this email directly or view it on GitHub
#1219 (comment).

@fatg3erman

This comment has been minimized.

Contributor

fatg3erman commented Jul 21, 2015

OK, added a test for the case when last_modified is 0.

jodal added a commit that referenced this pull request Jul 22, 2015

@jodal jodal merged commit 1d77bc7 into mopidy:develop Jul 22, 2015

3 checks passed

Scrutinizer 2 updated code elements
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.1%) to 75.605%
Details
@jodal

This comment has been minimized.

Member

jodal commented Jul 22, 2015

I merged and simplified the tests to not repeat too much from the previous test in commit 6cb48f2.

@fatg3erman fatg3erman deleted the fatg3erman:fix/1218-last-modified-for-mpd branch Jul 28, 2015

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