MTV Charts

[BlackJack]

@Dav1d: Du machst das unnötig kompliziert mit dem `isinstance()`. Das kannst Du Dir sparen und einfach *alles* kodieren. Bei `str`-Objekten die nichts ausserhalb von ASCII enthalten macht das nämlich keinen Unterschied. Genau deswegen kann die ElementTree-API auch in diesen Fällen `str` zurück geben, ohne dass das komisch sein muss.

[Dav1d]

@BlackJack, das geht nicht `position` und `last_position` sind beides Integer (Ich könnte `unicode` mit `basestring` ersetzen, aber das macht hier keinen Unterschied).

[BlackJack]

@Dav1d: Dann kodierst Du IMHO an der falschen Stelle.

[lunar]

@Dav1d: Ich hätte noch ein bisschen was zu verbessern:

Die Logik zum Finden der Tabelle ist doppelt vorhanden, und sollte in eine eigene Funktion ausgelagert werden. Mit CSS-Selektoren ließe sich das Finden der Tabelle auch in einer Zeile erledigen, ohne „magische“ Index-Zugriffe und die Generatorausdrücke im Schleifenkopf. Die Ergebnislisten, an die mit ".append()" angehängt wird, sollten durch Generatorfunktion ersetzt werden, die einzelne Ergebnisse sofort per "yield" zurückgeben. Die Existenz von "texttable" sollte geprüft werden, bevor die Seite abgerufen und geparst wird, damit der Nutzer nicht warten muss, nur um am Ende eine Fehlermeldung zu sehen.

Bei den Generatorausdrücken in den Schleifen, oder bei der letzten Zeile von "main()" hatte ich das Gefühl, dass Du da „cool“ programmieren und möglichst viel in möglichst wenig Zeichen und Zeilen ausdrücken wolltest. Das mag jetzt subjektiv und kleinkariert sein, doch ich finde, dass die Erstellung eines Wörterbuchs, der Index-Zugriff darauf und dann noch der Aufruf der gewählten Funktion zu viel ist für eine Zeile (letzte Zeile von "main()"). Du kommst nicht um, wenn Du jeden dieser Teile in einer Zeile separat an einen Namen bindest… es sieht zwar nicht ganz so schick aus, ist aber ungemein leichter zu verstehen. Gleiches gilt für die Inline-Funktion "make_table()", die eigentlich nicht innerhalb von "main()" definiert werden muss, sondern ebenso gut auf Modul-Ebene stehen kann, oder die Generator-Ausdrücke in den Schleifenköpfen, die Du auch in der Zeile vorher an einen Namen binden könntest.

Ich habe mir erlaubt, Dein Skript entsprechend diesen Anmerkungen zu überarbeiten: https://gist.github.com/3156407

[Hyperion]

lunars Script ergibt jetzt die korrekte Ausgabe:

Code: Alles auswählen

         4 |          6 | Tacabro                   | Tacatá 

Ich würde für dieses Script noch eine "reverse" Option einbauen, damit man nicht erst 100 Zeilen hochscrollen muss - oder eine TOP10?

[Dav1d]

Die Logik zum Finden der Tabelle ist doppelt vorhanden, und sollte in eine eigene Funktion ausgelagert werden. Mit CSS-Selektoren ließe sich das Finden der Tabelle auch in einer Zeile erledigen, ohne „magische“ Index-Zugriffe und die Generatorausdrücke im Schleifenkopf.

Stimmt (ich wusste nicht, dass die CSS-Selektoren von lxml so mächtig sind, scheinen aber das Gleiche wie z.B. jQuery zu beherrschen).

Die Existenz von "texttable" sollte geprüft werden, bevor die Seite abgerufen und geparst wird, damit der Nutzer nicht warten muss, nur um am Ende eine Fehlermeldung zu sehen.

Klar, macht sinn

Bei den Generatorausdrücken in den Schleifen, oder bei der letzten Zeile von "main()" hatte ich das Gefühl, dass Du da „cool“ programmieren und möglichst viel in möglichst wenig Zeichen und Zeilen ausdrücken wolltest. Das mag jetzt subjektiv und kleinkariert sein, doch ich finde, dass die Erstellung eines Wörterbuchs, der Index-Zugriff darauf und dann noch der Aufruf der gewählten Funktion zu viel ist für eine Zeile (letzte Zeile von "main()").

Hehe, es sollte eher schnell gehen, aber du hast natürlich recht.

Gleiches gilt für die Inline-Funktion "make_table()", die eigentlich nicht innerhalb von "main()" definiert werden muss, sondern ebenso gut auf Modul-Ebene stehen kann, oder die Generator-Ausdrücke in den Schleifenköpfen, die Du auch in der Zeile vorher an einen Namen binden könntest.

Ich wollte den module-scope nicht mit einer Funktion, die eigentlich nichts mit dem Modul zu tun hat "belasten", außerdem konnte ich direkt auf `parser` zugreifen. Aber wenn ich im Nachhinein darüber nachdenke, macht die Funktion doch auch im Modul Sinn.

Ich habe mir erlaubt, Dein Skript entsprechend diesen Anmerkungen zu überarbeiten: https://gist.github.com/3156407

Danke, leider kann man gists nicht mergen .

[Dav1d]

So, jetzt gibt es auch eine `--number` und `--reversed` Option.

Was sagt ihr zu Zeile 114?:

Code: Alles auswählen

charts = (next(all_charts) for _ in xrange(args.number))

Ich könnte natürlich auch eine Liste daraus machen und anschließend "slicen":

Code: Alles auswählen

charts = list(all_charts)[:args.number]

[BlackJack]

@Dav1d: Oder `itertools.islice()` verwenden.

[Dav1d]

Ah ja, oder so (geändert).

[Leonidas]

Ich habe mir erlaubt, Dein Skript entsprechend diesen Anmerkungen zu überarbeiten: https://gist.github.com/3156407

Danke, leider kann man gists nicht mergen .

Huch? Na klar kann man gists mergen. Einfach beide gists klonen und mergen, seh gar nicht wo dein Problem ist.

[derdon]

Ich würde für dieses Script noch eine "reverse" Option einbauen, damit man nicht erst 100 Zeilen hochscrollen muss - oder eine TOP10?

Dafür gibt es doch entsprechende shell-Tools: tac und tail.

[Hyperion]

Ich präferiere ja irgend wie eine direkte Funktionalität eines Programms, als bei solchen trivial Geschichten erst mit Pipes arbeiten zu müssen

Aber Danke für `tac` - das kannte ich noch nicht.

Leider scheitert das ganze:

Code: Alles auswählen

nelson@destiny ~/src/Python/forum/mtv % ./lunar_charts.py | tac
Traceback (most recent call last):
  File "./lunar_charts.py", line 112, in <module>
    main()
  File "./lunar_charts.py", line 108, in main
    print(format_charts(charts))
UnicodeEncodeError: 'ascii' codec can't encode character u'\xe1' in position 874: ordinal not in range(128)

Komisch, da der Aufruf ohne Piping klappt... und vor allem: Wie kann der nachgelagerte Prozess da störend wirken?

Bei lunars Lösung klappt der JSON-Export nicht, da man Generatoren wohl nicht nutzen darf:

Code: Alles auswählen

nelson@destiny ~/src/Python/forum/mtv % ./lunar_charts.py -o json
Traceback (most recent call last):
  File "./lunar_charts.py", line 112, in <module>
    main()
  File "./lunar_charts.py", line 108, in main
    print(format_charts(charts))
  File "/usr/lib/python2.7/json/__init__.py", line 231, in dumps
    return _default_encoder.encode(obj)
  File "/usr/lib/python2.7/json/encoder.py", line 201, in encode
    chunks = self.iterencode(o, _one_shot=True)
  File "/usr/lib/python2.7/json/encoder.py", line 264, in iterencode
    return _iterencode(o, 0)
  File "/usr/lib/python2.7/json/encoder.py", line 178, in default
    raise TypeError(repr(o) + " is not JSON serializable")
TypeError: <generator object _parse_charts at 0x7fc2968e1e10> is not JSON serializable

Also schnell mittels `list` eine Liste draus machen:

Code: Alles auswählen

    print(format_charts(list(charts)))

[BlackJack]

@Hyperion: Ohne Pipe kann Python die Kodierung erraten, die die Shell erwartet. Pipes beziehungsweise das Programm dahinter haben aber keine Kodierung die man erraten könnte, also wird ASCII angenommen. Das gleiche gilt für Umleitungen in eine Datei auf Shell-Ebene. Das passiert immer wenn man `unicode` mit ``print`` ausgibt ohne es selbst explizit zu kodieren. Das ist IMHO ein Fehler im Programm.

[Hyperion]

@BlackJack: Ah, also besteht tatsächlich ein Unterschied, ob ich eine Ausgabe in eine Shell mach oder in eine Pipe. Interessant, wusste ich noch nicht. Ich dachte Standard out == Standard out - da lag ich wohl falsch.

Die API von `texttable` empfinde ich aber auch als verwirrend:

Code: Alles auswählen

    for chart in charts:
        table.add_row([unicode(chart[k]).encode('utf-8')
                       for k in header])

Beim Überfliegen hab ich nur gesehen, dass man dort Bytestrings übergibt und daraus irrtümlich geschlossen, dass `table.draw` dann ebenfalls einen Bytestring liefert. Dem ist aber nicht so, sondern man bekommt Unicode zurück. Also muss ich meinen Patch noch mal patchen

Code: Alles auswählen

    print(format_charts(list(charts)).encode("utf-8"))

Dann klappt's auch mit `tac`:

Code: Alles auswählen

nelson@destiny ~/src/Python/forum/mtv % ./lunar_charts.py | tac        
...

         3 |          3 | Carly Rae Jepsen          | Call Me Maybe                       
-----------+------------+---------------------------+------------------------------------
         2 |          5 | Loreen                    | Euphoria                            
-----------+------------+---------------------------+------------------------------------
         1 |          1 | Lykke Li                  | I Follow Rivers                     
===========+============+===========================+====================================
           |  position  |                           |                                     
 position  |    last    |          artist           |                title                

[Dav1d]

@Leonidas, daran habe ich nicht gedacht, ich habe nach einem "merge" Button gesucht.

@Hyperion, es gibt übrigens `--reversed` und `--number`.

[Hyperion]

@Hyperion, es gibt übrigens `--reversed` und `--number`.

Ja, habe ich schon gesehen; mir gings hier um das Prinzip. Und letztlich sind dabei ja noch zwei Fehler aufgedeckt worden

[Dav1d]

Überarbeitet.