Grooveshark API

[maxi_king_333]

Hi,

vielleicht kennen einige von euch schon den Musik-Streaming-Dienst Grooveshark.
Leider braucht man um Musik damit zu hören den Flashplayer, welcher, da proprietär, auf meinem System nichts zu suchen hat.
Ich habe daher die API in Python implementiert, sodass man nun Musik damit hören kann.
Musik herunterzuladen, außer zum Cachen ist laut den Grooveshak Terms of Service nicht erlaubt.
Hier gibts den Code.

Viele Grüße
Maxi

P.S. Ich hoffe ich bekomme hier ein bisschen mehr Feedback als bei meinem ctypes-Fluidsynth-Wrapper

[lunar]

@maxi_king_333: Zumindest rudimentäre Dokumentation ist eigentlich obligatorisch, wenn Du möchtest, dass sich irgendjemand den Quelltext ansieht, oder gar verwendet. Insbesondere ist die Lizenz keine Modul-Dokumentation, sollte also auch nicht im Docstring stehen. Da gehört eigentlich eine Beschreibung hin, die erklärt, wofür das Modul da ist und wie man es verwendet.

In "setup.py" könnte man mehr Metadaten hinzufügen, insbesondere Name und Mailadresse des Autoren, damit man weiß, an wen man sich wenden kann.

Das Aktualisieren der Exemplar-Attribute mit JSON-Daten (beispielsweise in "Album", "Artist", oder "PlayList") ist unschön. Das kann man eleganter lösen, entweder indem man über die Schlüssel des Wörterbuchs iteriert und die Attribute dann dynamisch mit "setattr()" zuweist, oder – was ich bevorzuge würde – in dem man das Wörterbuch direkt als "self._data" speichert, und anschließend Eigenschaften zum Zugriff auf die einzelnen Attribute implementiert. Die könnte man bei der Gelegenheit dann auch gleich mit Docstrings versehen. Die API könnte man dabei auch verschönern, indem man "Album.artist" nicht als irgendein magisches Tupel setzt, sondern als Eigenschaft implementiert, die anhand ID des Künstlers die Metadaten des Künstlers vom Server abfragt, und dann ein anständiges "Artist"-Objekt zurückgibt. Dann könnte man auf den Namen des Albumkünstlers per "album.artist.name" zugreifen und nicht per "album.artist[0]", wo man immer erst nachsehen muss, was sich hinter der 0 verbirgt.

Das zum Quelltext, genauer habe ich nicht hingesehen, weil ich den Dienst nicht kenne, und Dokumentation wie gesagt fehlt. Insofern kann ich so direkt überhaupt gar nicht jeden Teil des Quelltexts verstehen.

Die Einschränkungen aus der README, dass das Modul nur für "educational use" verwendet werden darf, geht nicht mit der GPL zusammen. Entweder GPL oder proprietär, aber GPL mit Nebenbedingungen geht nicht.

PS: Das mit der Dokumentation gilt übrigens auch für Dein anderes Modul. Vielleicht hast Du deswegen keine Rückmeldung bekommen, weil niemand bereit ist, einen vollkommen undokumentierten Quelltext zu lesen. fluidsynth ist ja auch nicht gerade eine Mainstream-Software, mit der fast jeder schon mal gearbeitet hat. Im Gegenteil, die Anzahl der Nutzer einer solchen Bibliothek ist in diesem Forum wahrscheinlich eher gering, also musst Du uns halt auch ein bisschen helfen, indem Du Deine Module gut dokumentierst.

[maxi_king_333]

Danke, für die konstruktive Kritik.

Zumindest rudimentäre Dokumentation ist eigentlich obligatorisch, wenn Du möchtest, dass sich irgendjemand den Quelltext ansieht, oder gar verwendet. Insbesondere ist die Lizenz keine Modul-Dokumentation, sollte also auch nicht im Docstring stehen. Da gehört eigentlich eine Beschreibung hin, die erklärt, wofür das Modul da ist und wie man es verwendet.

Mir war bisher garnicht so bewusst, dass die ''' zur Dokumentation dienen.
Ich habe mir mal eines Deiner Module angeguckt, https://github.com/lunaryorn/pyudev/blo ... _init__.py.
Dort verwendest Du ja reStructuredText.
Mich würde mal interressieren (wenn schon dann richtig), ob bzw. wie man den aus dem Quellcode in z.B. ein HTML Dokument bekommt.
Dann werde ich die Dokumentation hinzufügen.

Das Aktualisieren der Exemplar-Attribute mit JSON-Daten (beispielsweise in "Album", "Artist", oder "PlayList") ist unschön. Das kann man eleganter lösen, entweder indem man über die Schlüssel des Wörterbuchs iteriert und die Attribute dann dynamisch mit "setattr()" zuweist, oder – was ich bevorzuge würde – in dem man das Wörterbuch direkt als "self._data" speichert, und anschließend Eigenschaften zum Zugriff auf die einzelnen Attribute implementiert.

Was meinst Du mit Eigenschaften? Methoden?

Die API könnte man dabei auch verschönern, indem man "Album.artist" nicht als irgendein magisches Tupel setzt, sondern als Eigenschaft implementiert, die anhand ID des Künstlers die Metadaten des Künstlers vom Server abfragt, und dann ein anständiges "Artist"-Objekt zurückgibt. Dann könnte man auf den Namen des Albumkünstlers per "album.artist.name" zugreifen und nicht per "album.artist[0]", wo man immer erst nachsehen muss, was sich hinter der 0 verbirgt.

Dann bräuchte ich halt sehr viele Requests.
Wenn ich z.B. nach "xyz" suche und nun sagen wir 100 Songs bekommen, dann müsste ich für jeden dieser 100 Songs ein Artist und ein Album Objekt erzeugen, was 2 * 100 Requests nötig macht.
Ich sollte vielleicht einfach 2 Namen vergeben, artist_name und artist_id zum Beispiel... wäre das besser?

Die Einschränkungen aus der README, dass das Modul nur für "educational use" verwendet werden darf, geht nicht mit der GPL zusammen. Entweder GPL oder proprietär, aber GPL mit Nebenbedingungen geht nicht.

Weiß auch nicht, wie ich darauf kam - ist entfernt.

PS: Das mit der Dokumentation gilt übrigens auch für Dein anderes Modul. Vielleicht hast Du deswegen keine Rückmeldung bekommen, weil niemand bereit ist, einen vollkommen undokumentierten Quelltext zu lesen.

Werde ich bei Gelegenheit dann auch nachholen.

Viele Grüße
Maxi

[lunar]

@maxi_king_333: Du kannst epydoc oder Sphinx verwenden, um aus den Docstrings HTML-Dokumentation zu erzeugen. Ich nutze letzteres, die dazu nötige Konfiguration und die entsprechenden ReST-Dokumente findest Du im doc/ Verzeichnis. Das enthält die gesamte Website des Projekts.

Mit Eigenschaften meine ich nicht Methoden, sondern eben Eigenschaften. Im Unterschied zu Attributen werden Eigenschaften erst beim Zugriff ausgewertet, und können Docstrings enthalten. Bei der Verwendung einer Eigenschaft für "album.artist" hättest Du auch nicht das Problem unnötiger Requests, weil der Request dann eben erst dann stattfindet, wenn Du wirklich auf "album.artist" zugreifst.

[maxi_king_333]

Vielen Dank, werde mich mit der Dokumentation mal ein bisschen beschäftigen.
Dieses Sphinx scheint eine gute Sache zu sein, da habe ich dann neben den Kommentaren gleich noch eine schöne HTML-Dokumentation.

Mit Eigenschaften meine ich nicht Methoden, sondern eben Eigenschaften. Im Unterschied zu Attributen werden Eigenschaften erst beim Zugriff ausgewertet, und können Docstrings enthalten. Bei der Verwendung einer Eigenschaft für "album.artist" hättest Du auch nicht das Problem unnötiger Requests, weil der Request dann eben erst dann stattfindet, wenn Du wirklich auf "album.artist" zugreifst.

Hmm... schein zwar eine praktikable Sache zu sein, jedoch ist das Problem unötiger Requests damit nicht gelöst.
Wenn ich suche, dann möchte ich auch eine Liste mit Resultaten bekommen, wozu ich dann auch wieder die Requests brauche.
Ich glaube die von Grooveshark haben sich schon was dabei gedacht den Namen des Künstlers dem Song anzuheften.
Die magischen Nummern sind trotzdem unschön, ich werde einfach zwei Eigenschaften dafür definieren.

Viele Dank und Viele Grüße
Maxi

[lunar]

@maxi_king_333: Nun, Du kannst die Eigenschaften ja auch so implementieren, dass "album.artist" ein Artist-Objekt zurückgibt, dass den Namen und die ID vom Album übernimmt. Wie genau Du das implementierst, kannst Du Dir selbst überlegen, es ist eben nur so, dass "album.artist[0]" eine unschöne API ist.

Gerade bei einem "Song"-Objekt wird das dann auch echt umständlich, wenn man beispielsweise über das Album des Lieds an die anderen Lieder dieses Albums herankommen möchte. Schön wäre "for song in some_other_song.album.songs:", bei Dir muss man folgendes tun:

Code: Alles auswählen

album_name, album_id = some_other_song.album
album = Album(some_other_song.connection, some_other_song.connection.get_album_by_id(album_id))
for song in album.get_songs():

Du siehst den Unterschied, oder?

Beim Neuentwurf dieser Klassen könntest Du auch "Connection" umschreiben. Momentan ist das eine Art "Super-Objekt", dass Hilfsmethoden für diverse Abfragen enthält. Auch das ist nicht gelungen, denn eine allgemeine Verbindung muss nicht wissen, wie man die Lieder eines Albums abfragt. Das gehört eigentlich in die Album-Klasse. Gleiches gilt für alle anderen ".get_*()"-Methoden der "Connection"-Klasse.

[maxi_king_333]

Hi,

hat ein bisschen länger gedauert.
Ich glaube aber, dass ich das mit der Dokumentation dafür jetzt verstanden habe.
Die Dokumentation habe ich hier.
Ist aber irgendwie seltsam, dass die Stylesheets nicht geladen werden...

Auch den Code habe ich überarbeitet, die API ist jetzt so, wie von Dir vorgeschlagen.

Was meinst Du/meint Ihr dazu?

Viele Grüße
Maxi

[derdon]

Du hast die Datei .nojekyll vergessen. Daher wird keine Datei aus einem Ordner gefunden, der mit _ beginnt.Für Details und Erklärungen, siehe http://pages.github.com/, Abschnitt “Using Jekyll For Complex Layouts”. (Warum sind nur so viele Seiten zu faul/blöd, Ankerlinks zu benutzen?)

[lunar]

@maxi_king_333: Der Quelltext aller ".songs"-Eigenschaften ist bis auf die Anfrage in der ersten Zeile vollkommen identisch, und sollte dementsprechend in eine Hilfsfunktion ausgelagert werden. Ich würde eine Hilfsfunktion ala "Song.from_response()" implementieren, die aus einem einzelnen "Song" ein entsprechendes Objekt erzeugt. Die Songs-Eigenschaften würde ich dann nicht als Liste, sondern als Generator implementieren, so dass die Lieder wirklich nur abgefragt werden, wenn sie benötigt werden. Mit der Hilfsfunktion wäre das dann einfach "return (Song.from_response(s) for s in songs)".

Die einem Song zugeordneten Objekte würde ich auch nicht vorweg erzeugen, sondern erst in den Eigenschaften. "CoverArtFilename" beispielsweise einfach als "Album._cover_art_filename" speichern, und erst in "Album.cover" daraus ein Picture-Objekt erzeugen:

Code: Alles auswählen

@property
def cover(self):
    return Picture(PICTURE_URL % self._cover_art_filename)

Die URL, die zum Zugriff auf das Cover verwendet wird, könnte man an dieser Stelle dann gleich auch als Konstante definieren, damit man sie nur einmal anpassen muss, wenn Grooveshark mal die API ändert.

Datei-Objekte jeglicher Art sollte man immer schließen. "Picture.data" sollte also so aussehen:

Code: Alles auswählen

request = urllib2.Request(...)
with closing(urllib2.urlopen(request)) as response:
    self._data = response.read()

In "Album.id" ist eine Endlosrekursion, da "Album.id" wieder auf "Album.id" zugreift. Dort hast Du den führenden Unterstrich "_id" vergessen.

[maxi_king_333]

Hi,
hat ein bisschen länger gedauert - stressige Woche.

@derdon
Danke, jetzt funktioniert es.
Unter dem Abschnitt "Using Jekyll For Complex Layouts" habe ich das jedoch nicht erwartet.

(Warum sind nur so viele Seiten zu faul/blöd, Ankerlinks zu benutzen?)

Ähmm... ich benutze das erst seit kurzem.
Du meinst doch, dass z.B die Namen der Klassen mit der Klasse verlinkt sind, oder?
Ich mache die Doumentation ja über dieses Autogen-Teil.
Ich benutze dazu :class:, wenn ich es richtig gelesen habe, sollte :py:class: das machen: http://sphinx.pocoo.org/domains.html#python-roles.
Jedoch geht das irgendwie nicht. - Ich habe es exemplarisch ausprobiert.

@lunar
Ich habe versucht das so umzusetzen, ist es mir gelungen?

In "Album.id" ist eine Endlosrekursion, da "Album.id" wieder auf "Album.id" zugreift. Dort hast Du den führenden Unterstrich "_id" vergessen.

Ich finde es genial, dass Du soetwas siehst, wo es mir selber nicht auffällt.

Viele Grüße
Maxi

[BlackJack]

@maxi_king_333: derdon meinte mit der Bemerkung in Klammern die Github-Seite und nicht Dich. Anker(links) sind Linkziele innerhalb einer Seite, damit man nicht nur auf die Seite als ganzes, sondern auch auf bestimmte Stellen darin direkt verweisen kann. Also zum Beispiel auf Überschriften. Beispielsweise direkt den Abschnitt zur If-Anweisung im Python-Tutorial statt nur die Seite verlinken zu können und dazu sagen zu müssen, dass man bis „4.1. if Statements” runterscrollen muss.

[maxi_king_333]

@maxi_king_333: derdon meinte mit der Bemerkung in Klammern die Github-Seite und nicht Dich. Anker(links) sind Linkziele innerhalb einer Seite, damit man nicht nur auf die Seite als ganzes, sondern auch auf bestimmte Stellen darin direkt verweisen kann. Also zum Beispiel auf Überschriften. Beispielsweise direkt den Abschnitt zur If-Anweisung im Python-Tutorial statt nur die Seite verlinken zu können und dazu sagen zu müssen, dass man bis „4.1. if Statements” runterscrollen muss.

Achso, danke...

[lunar]

@maxi_king_333: In "Album.songs" wird der komplette Response in "Album._songs" gespeichert, der Generator mit Song-Objekten aber jedes Mal neu erzeugt. Da kann man die Objekte auch gleich in einer Liste speichern, und einen Iterator über diese Liste zurückgeben:

Code: Alles auswählen

if self._songs is None:
    response = self._connection.request(…)
    self._songs = [Song.from_request(song, self._connection) for song in response]
return iter(self._songs)

Es ist auch sinnvoll, den Vergleich explizit gegen None durchzuführen, da ansonsten ein leerer Response immer wieder neu angefordert wird. Es ist zwar unwahrscheinlich, dass ein Album mal keine Lieder enthält, aber sicher ist sicher

Das gilt auch für einige andere Eigenschaften (e.g. Artist.similar), ansonsten sieht der Quelltext jetzt ganz gut aus.

Nachtrag: Noch ein paar Anmerkungen zu Deinen Commits. Du wirst in Deinen Commits (zumindest im letzten "0e9bf222d0ff21db6b09", in dem Du die API geändert hast) zu viele Änderungen zusammen. Ein Commit sollte immer nur eine einzige diskrete Änderung am Quelltext enthalten. Der erwähnte Commit enthält mindestens drei, nämlich das Refactoring, die Korrektur von Album.id und das Verschieben der Cover-URL in eine Konstante, alles unabhängige Änderungen. Solche Änderungen sind für Dritte nur schwer nachzuvollziehen, insbesondere wenn wichtige Änderungen wie die Korrektur von "Album.id", die im großen Diff nicht auffallen, in der Commit-Nachricht nicht erwähnt sind.

Die Commit-Nachrichten sollten den Inhalt und den Zweck einer Änderung vollständig erläutern. Bei einem Commit zur Korrektur von "Album.id" wäre "Fixed attribute name in Album.id" genug, aber "API changed" reicht definitiv nicht als Änderungskommentar für das umfangreiche Refactoring. Nach einem einleitenden Satz sollten bei dieser Änderung mindestens zwei Absätze folgen, der erste mit Erläuterung zur Änderung, der zweite mit dem Grund dafür. Eine dritte Person, die zwar mit Quelltext und Projekt, aber nicht mit Deinen Änderungen vertraut ist, sollte den Inhalt und Anlass eines Commits aus der Nachricht ablesen und verstehen können, ohne das Diff betrachten oder Dir Rückfragen stellen zu müssen.

Ich wäre im Bezug auf den erwähnten Commit so vorgegangen: Zuerst "Album.id" korrigieren, und committen. Dann die URL in eine Konstante verschieben und wieder commiten. Nun einen neuen Zweig (e.g. "refactoring") erzeugen, in diesen Zweig wechseln, und dann jede Klasse umarbeiten und anschließend commiten, und zwar mit umfangreichen Commit-Nachrichten, welche die Änderungen und auch den Grund dazu beschreiben. So ist dieser Zweig zwar die meiste Zeit nicht lauffähig, aber bei einem Entwicklungszweig ist das ja egal. Anschließend testen, und erst dann das refactoring in den master zusammenführen und pushen.

Das erleichtert nicht nur das Nachvollziehen der Änderungen, sondern zwingt Dir selbst im Entwicklungsprozess mehr Struktur auf, was wiederum viele Flüchtigkeitsfehler verhindert.

[maxi_king_333]

@maxi_king_333: In "Album.songs" wird der komplette Response in "Album._songs" gespeichert, der Generator mit Song-Objekten aber jedes Mal neu erzeugt. Da kann man die Objekte auch gleich in einer Liste speichern, und einen Iterator über diese Liste zurückgeben:

Die Songs-Eigenschaften würde ich dann nicht als Liste, sondern als Generator implementieren, so dass die Lieder wirklich nur abgefragt werden, wenn sie benötigt werden

Moment?
Ich dachte, ich hätte das so gemacht, wie Du anfangs gemeint hast..
Wie hast Du das dann gemeint?

Habe jetzt mal versucht den "is None"-Commit besser zu beschreiben...
Ganz schön kompliziert das ganze...

[lunar]

@maxi_king_333: Einen Generator kannst Du verwenden, wenn Du gar nichts im Exemplar speicherst, sondern jedes Mal beim Zugriff auf die Eigenschaft einen Request durchführst. Dann werden die Daten nur bei Bedarf geparst:

Code: Alles auswählen

@property
def songs(self):
    return (Song.from_response(s) for s in self._connection.request(…))

Du dagegen speicherst in Deinen Eigenschaften den Response:

Code: Alles auswählen

@property
def songs(self):
    if self._songs is None:
         self._songs = self._connection.request(…)
    return (Song.from_response(s) for s in self._songs)

Das ist per se eine gute Idee, um Netzwerkverkehr zu vermeiden (die Lieder eines Albums ändern sich ja sowieso nie). Aber es ist eigentlich nicht sinnvoll, nur die Rohdaten zu speichern und das Parsen jedes Mal aufs Neue durchzuführen. Stattdessen kann man einfach das Endresultat speichern, und spart sich das erneute parsen:

Code: Alles auswählen

@property
def songs(self):
    if self._songs is None:
         self._songs = [Song.from_response(s) for s in self._connection.request(…)]
    return iter(self._songs)

Ein Generator ist generell etwas, was nur bei Bedarf Ergebnisse produziert. Du aber hast Dich entschieden, Deine Eigenschaften so zu implementieren, dass das Ergebnis nur beim ersten Zugriff abgefragt, und dann gecacht wird. Dann ist es sinnvoller, gleich das vollständig verarbeitete Ergebnis zu cachen. Ich hoffe, dass hilft Dir. Verzeih die Verwirrung.

Beim Schreiben des Beitrags ist mir aufgefallen, dass die "from_request()"-Methoden besser ".from_response()" heißen sollten, denn sie verarbeiten ja den Response, der auf den bereits über "connection" gestellten Request folgt.

[maxi_king_333]

@lunar
Danke, für die Erklärung, habe es angepasst...
Und generell einmal Vielen Dank, für Deine Hilfe und Geduld.

Jetzt wo das Modul akzeptabel ist, vielleicht möchte jamand eine Oberfläche draufsetzen?

[weißned]

Wie hast du denn heraus gefunden, dass das genau so funktioniert? Gib es da eine Dokumentation zu? Mich würde nämlich mal interessieren, wie man so ein Modul schreibt, aber verstehe dein Modul im Moment noch etwas weniger

Wäre cool wenn du mir da etwas Hilfestellung geben könntest.

[waki]

Hab mir das gerade mal angekuckt und finde das eigentlich ganz geil ^^ Ich wär nur sehr dankbar wenn jemand eine windows-freundliche abspiel methode des streams hätte, bekomm das gerade einfach nicht hin ^^ Kann mir jemand helfen? ^^

[weißned]

Kommt darauf an, wie genau du das realisieren willst.

[waki]

meinst du mich? ich würde einfach nur gerne den Stream abspielen, was ich jedoch nicht hinbekomme, da die meisten audio-libs eine Datei zum abspielen wollen...