Script Optimierung: getopts, argumente, fehlerhandling, etc.

[think]

Hallo miteinander,

Ich habe ein kleines Script geschrieben, welches ich - salopp gesagt - aus einem getopts-Beispiel und meinem ursprünglichen Script zusammengesetzt habe. Nun habe ich dazu ein paar Fragen und hoffe ihr könnt mir helfen und vielleicht ein paar allgemeine Tipps geben. :-)

Hier das Script. Bei Linien zu welchen Fragen habe oder Tipps brauchen könnte, hab ich einen Kommentar angehängt. Grundsätzlich sei noch gesagt, dass das Script in der derzeitigen Aussführung funktioniert. Mir geht es also um allgemeine Optimierung bzw. Fehlerquellen.:

Code: Alles auswählen

#!/usr/bin/env python
# -*- coding: utf-8 -*-

import getopt, sys, GeoIP

def resolveAdress(args = sys.argv): # diese Funktion habe ich selbst gebaut. Kann man den Aufbau verbessern, gibt es irgendwelche "Fehler"?
    gi = GeoIP.new(GeoIP.GEOIP_MEMORY_CACHE)
    try:
        print gi.country_code_by_addr(args[2]) # Hier die Frage, ob man dieses Konstrukt besser lösen kann, als mit args[2] und try?
    except Exception, err: # Fehlerbehandlung: Kann man dies vielleicht "verbessern"?
        print str(err)
        usage()
        sys.exit(2)

# Alles hier drunter habe ich kopiert und auf meine Bedürfnisse angepasst

def usage(args = sys.argv):
    print 'usage: %s [OPTIONS]' % args[0]
    print ''
    print 'options'
    print ''
    print '  -a, --adress A.B.C.D'
    print '  -h, --help    Show this help menu'
    print ''

def main():
    try:
        opts, args = getopt.getopt(sys.argv[1:], "ha", ["help", "adress"])
    except getopt.GetoptError, err:
        print str(err)
        usage()
        sys.exit(2)

    for o, a in opts:
        if o in ("-h", "--help"):
            usage()
            sys.exit()
        elif o in ("-a", "--adress"):
            resolveAdress()
        else:
            assert False, "unhandled option"

if __name__ == "__main__":
    main()

Unabhängig zur Funktion die ich selbst gebaut hab, ist die Frage ob man am ganzen Aufbau noch was verbessern kann. Ich möchte das Script also generell optimieren, damit ich mich nicht schämen muss, wenn ich das irgendwem zur Benutzung gebe. :-)

Ich erwarte nicht, dass jemand das Script komplett optimiert und auf den Kopf stellt. Wenn ihr irgendwelche Linktipps habt, bin ich damit auch zufrieden.

Wer das Script lokal ausführen will, muss dazu das Paket python-geoip installieren (Ubuntu, Debian). Wie es bei anderen Distributionen oder gar bei Windows heisst, weiss ich nicht.

Grüsse,
think

[cofi]

1. Nutze argparse (oder zumindest optparse), `usage` wird damit u.a. ueberfluessig.

2. Schreibe deine Funktionen nicht so, dass sie die Argumente noch einmal selbst auswerten muessen, gib sie ihnen einfach.

3. `-a` ist keine Option. Dein Programm tut nichts anderes und ohne tut es auch nichts.

4. `assert False, ..`, du kannst auch einfach so `print` nutzen.

5. Eine Funktion sollte nicht einfach das Programm beenden, lass den Aufrufenden Code die Situation/Ausnahme behandeln

6. Die Imports sollten auf eigenen Zeilen sein; trenne zumindest Stdlib und 3rd Party libs

[think]

Edit: Hier meine "Gedanken" zu deinen Punkten:

1. Nutze argparse (oder zumindest optparse), `usage` wird damit u.a. ueberfluessig.

argparse sieht schonmal sehr gut aus. Danke für diesen Tipp.

2. Schreibe deine Funktionen nicht so, dass sie die Argumente noch einmal selbst auswerten muessen, gib sie ihnen einfach.

Also mache die Überprüfung in der main() und gebe das übergeprüfte Argument dann der Funktion?

3. `-a` ist keine Option. Dein Programm tut nichts anderes und ohne tut es auch nichts.

Kannst du das evt. präzisieren. Denn die Funktion ist zumindest gegeben:

Code: Alles auswählen

$ python dh-geo-getopts.py 8.8.8.8 #Ergibt keine Ausgabe
$ python dh-geo-getopts.py -a 8.8.8.8 # Ergibt die korrekte Ausgabe:
US

4. `assert False, ..`, du kannst auch einfach so `print` nutzen.

Ok.

5. Eine Funktion sollte nicht einfach das Programm beenden, lass den Aufrufenden Code die Situation/Ausnahme behandeln

Also per return Wert von der Funktion beenden lassen?

6. Die Imports sollten auf eigenen Zeilen sein; trenne zumindest Stdlib und 3rd Party libs

Ok.

Danke dir jedenfalls. :-)

[BlackJack]

@think: Zu 2.: Genau. Die Funktion solltest Du so schreiben, dass man sie auch ohne Kenntnis des Programms verwenden kann. Also keine Liste mit Argumenten übergeben wo die Funktion dann wissen muss an welchem Index in den Argumenten von der Kommandozeile das steht was eigentlich interessiert. Und statt das Ergebnis auszugeben, solltest Du auch den Wert an den Aufrufer zurück geben (wie Du bei 5. ja schon selber bemerkst). Damit wäre die Funktion unabhängiger benutzbar und nicht so stark mit dem Rest des Programms verwoben. Und zum Beispiel auch automatisiert testbar.

Von `getopt` sollte man ja sowieso weg kommen — die Alternativen wurden ja schon genannt — aber vielleicht doch noch die Bemerkung, dass Du es falsch benutzt: Die Beschreibung der Optionen sollte für `-a`/`-adress` (sic!) ein Argument vorsehen dass dann in der Schleife über `opts` an `a` gebunden wird. Dann ist es völlig egal an welchem Index in `sys.args` das Argument mal stand, oder ob der Benutzer das Programm zum Beispiel mit ``--adress=10.0.0.42`` aufruft — was nämlich eigentlich möglich sein sollte und bei Deinem Code nicht funktionieren würde.

Zu 3.: cofi meinte nicht die Kommandozeilen-API wie Du sie *jetzt* umgesetzt hast, sondern wie Du sie umsetzen solltest. Optionen heissen so, weil man damit die *optionalen* Werte festlegen *kann*. Dein Programm scheint zum funktionieren die Adresse aber zwingend zu benötigen. Wenn man das Programm also ohne eine Adressangabe nicht sinnvoll aufrufen kann, dann sollte die Adresse auch keine Option sondern ein Argument sein. In der `optparse`-Dokumentation sollte irgendwo ein Abschnitt über ”mandatory options” stehen, und dass das ein Widerspruch ist.

[think]

Vielen Dank für die Präzisierung und Tipps. Ich werde versuche das Script dahingehend umzubauen. Sollte ich dann auf weitere Probleme/Fragen stossen, melde ich mich wieder.

Danke.

[think]

Habe jetzt mein Script mit argparse umgeschrieben. Sieht im Moment folgendermassen aus:

Code: Alles auswählen

#!/usr/bin/env python
# -*- coding: utf-8 -*-

import GeoIP
import argparse

parser = argparse.ArgumentParser()
parser.add_argument('ip', help='IP to resolve')
args = parser.parse_args()

ip = '%s' % (args.ip)

def resolveAdress(ip):
    gi = GeoIP.new(GeoIP.GEOIP_MEMORY_CACHE)
    print gi.country_code_by_addr(ip)

def main():
    resolveAdress(ip)

if __name__ == "__main__":
    main()

Das Script funktioniert soweit, allerdings bin ich mir sicher, dass man vieles Optimieren kann. Deshalb hier meine Probleme/Fragen dazu:

1. Kann mir jemand ein Beispiel geben, wie ich die Argumente (richtig) in die main() Funktion einbaue. (Try, Catch habe ich extra weggelassen).
2. Gehört der argparse Teil auch in eine eigene Funktion oder wie baue ich diesen Teil "sauber" ein?
3. Sonstige Optimierungen?
4. Weiteres?

[BlackJack]

@think: Zu 1.: Ich verstehe die Frage nicht!?

Zu 2.: Würde ich mit in die `main()` stecken.

Zu 3.: Die Zeile mit der Definition von `ip` verstehe ich nicht? Warum eine Zeichenkette nochmal in eine Zeichenkette umwandeln und warum so kompliziert mit ``%``?

Ansonsten würde nach den üblichen Namenskonvention und dem Wörterbuch `resolveAdress()` eher `resolve_address()` heissen. Ich erwähnte ja schon mal dass ich die Funktion etwas zurück geben lassen würde, damit man sie auch verwenden kann, wenn man das Ergebnis nicht per ``print`` ausgeben lassen möchte.

Code: Alles auswählen

#!/usr/bin/env python
# -*- coding: utf-8 -*-
import argparse
import GeoIP


def resolve_address(ip):
    return GeoIP.new(GeoIP.GEOIP_MEMORY_CACHE).country_code_by_addr(ip)


def main():
    parser = argparse.ArgumentParser()
    parser.add_argument('ip', help='IP to resolve')
    args = parser.parse_args()
    print resolve_adress(args.ip)


if __name__ == '__main__':
    main()

[think]

1. Hat sich erübrigt. Was ich suchte ist bei dir ersichtlich.
2. Ok
3. Hatte ich so von einem argparse Tutorial kopiert.

Auf jeden Fall hilft mir deine Lösung extrem weiter. Ich hatte beim return Teil von der Funktion irgendwie eine Denkblockade. Die hast du jetzt gelöst.

Btw.: Bei dir hat sich bei "def resolve_address(ip):" ein Schreibfehler eingeschlichen.

Hier das voll funktionierende Script:

Code: Alles auswählen

#!/usr/bin/env python
# -*- coding: utf-8 -*-
import argparse
import GeoIP


def resolve_address(ip):
    return GeoIP.new(GeoIP.GEOIP_MEMORY_CACHE).country_code_by_addr(ip)


def main():
    parser = argparse.ArgumentParser()
    parser.add_argument('ip', help='IP to resolve')
    args = parser.parse_args()
    print resolve_address(args.ip)


if __name__ == '__main__':
    main()

Herzlichen Dank nochmal.

[BlackJack]

@think: Du solltest vielleicht mal ein Wörterbuch konsultieren. Das heisst wirklich "address" mit zwei 'd'.

[cofi]

@think: Du solltest vielleicht mal ein Wörterbuch konsultieren. Das heisst wirklich "address" mit zwei 'd'.

Gar nicht wahr! Wenn man sich aber auf den Standpunkt stellt, dass es richtig ist, sollte man den Rest des Quellcodes auch mit deutschen Bezeichnern versehen

[BlackJack]

@cofi: *hust* Auf Deutsch hiesse es aber "Adresse" — mit einem 'e' am Ende.

[think]

@think: Du solltest vielleicht mal ein Wörterbuch konsultieren. Das heisst wirklich "address" mit zwei 'd'.

Ich war mir nicht sicher, da hab ich mich für ein d entschieden. Habs jedenfalls oben korrigiert.

[think]

Sorry, ich schon wieder...

Ich hab jetzt noch ein dict konstruiert, welches aus US -> United States, CH -> Switzerland macht.

Hier das Script (Ich habe das dict gekürzt, weils mehrere hundert Linien sind...):

Code: Alles auswählen

#!/usr/bin/env python
# -*- coding: utf-8 -*-

import argparse
import GeoIP

def resolve_address(ip):
    return GeoIP.new(GeoIP.GEOIP_MEMORY_CACHE).country_code_by_addr(ip)

def main():
    parser = argparse.ArgumentParser()
    parser.add_argument('ip', help='IP to resolve')
    args = parser.parse_args()
    iso_country_codes = {"AD" : "Andorra",
                                    "AE" : "United Arab Emirates",
                                    "AF" : "Afghanistan",
                                    "AG" : "Antigua And Barbuda",
                                    "AI" : "Anguilla",
                                    "AL" : "Albania",
                                    "AM" : "Armenia",
                                    "AN" : "Netherlands Antilles",
                                    "AO" : "Angola",
                                    "AQ" : "Antarctica",
                                    "AR" : "Argentina",
                                    "AS" : "American Samoa",
                                    "AT" : "Austria"}
    print iso_country_codes[resolve_address(args.ip)]

if __name__ == '__main__':
    main()

Jetzt wieder die Selbe Frage wie oben, wie optimieren?

Kann/Soll ich das dict und das drumherum in eine eigene Funktion packen oder ist das i.O. so? Wenn ichs in eigene Funktion packen soll, wie übergebe ich den CountryCode am besten an diese Funktion? Ich habs mit einigen Möglichkeiten versucht, kam allerdings nicht zum gewünschten Ergebnis.

[derdon]

Du suchst die library Babel und dazu dieses passende Beispiel: 2 The Locale Class.

Code: Alles auswählen

>>> from babel import Locale
>>> locale = Locale('en', 'US')
>>> locale.territories['US']
u'United States'
>>> locale.territories['AT']
u'Austria'
>>> locale.territories['AR']
u'Argentina'
>>> locale.territories['AQ']
u'Antarctica'
>>> locale.territories['AE']
u'United Arab Emirates'
>>> locale.territories['AG']
u'Antigua and Barbuda'

[think]

Danke, scheint zu passen. (Zumindest in einem Testscript). Ich baue das ganze jetzt noch in mein Script ein und paste es dann wieder hier rein.

So siehts im Moment aus. Habe mich für de_DE entschieden und es scheint zu klappen:

Code: Alles auswählen

#!/usr/bin/env python
# -*- coding: utf-8 -*-

from babel import Locale
import argparse
import GeoIP

def resolve_address(ip):
    return GeoIP.new(GeoIP.GEOIP_MEMORY_CACHE).country_code_by_addr(ip)

def main():
    parser = argparse.ArgumentParser()
    parser.add_argument('ip', help='IP to resolve')
    args = parser.parse_args()
    locale = Locale('de', 'DE')
    print locale.territories[resolve_address(args.ip)]

if __name__ == '__main__':
    main()

Auch hier die Frage, habe ich "locale = Locale('de', 'DE')", usw. an der richtigen Stelle integriert? Wenn Nein, wie kann ich es besser machen?

[derdon]

So, hab versucht, es etwas auszubessern: http://paste.pocoo.org/show/487048/. Wenn die Angaben von der Sprache und Ort nicht zusammen passen, gibt es eine Fehlermeldung wie hier (dafür sorgen die Zeilen 40-43):

Code: Alles auswählen

./tmp.py 1234 -l de -t US
usage: tmp.py [-h] [-l LANGUAGE] [-t TERRITORY] ip
tmp.py: error: unknown locale 'de_US'

Wird keine Angabe zu Sprache / Ort gemacht, wird auf die lokalen Einstellugen der Shell zurückgegriffen. Ich konnte es nicht komplett testen, weil ich das geoip Paket nicht installiert habe. Normalerweise lasse ich bei meiner parse_options gleich die `args` zurückgeben, aber wegen Zeile 43 wird ja ein ArgumentParser-Objekt benötigt, daher lasse ich ein parser-objekt zurückgeben.

Edit: Noch eine Anmerkung: Dass man bei der Funktion set_default_locale ein anderes Modul (oder genau genommen irgendein Objekt) übergeben kann als das locale-Modul aus der stdlib, liegt hauptsächlich daran, dass sie sich so leichter testen lässt (Stichwort testability). Ein netter Nebeneffekt ist außerdem, dass diese Funktion so schneller ist, weil auf lokale Namen schneller zugegriffen werden kann als auf globale Namen. Das ist aber nur bei sehr vielen Funktionsaufrufen relevant. Man sollte also noch dokumentieren, dass "der normale Benutzer" diese Funktion ohne Parameter aufruft; das habe ich vergessen.

[think]

Wenn man noch ein "Import GeoIP" anfügt funktioniert es perfekt.

Herzlichen Dank jedenfalls an alle Helfer!

[derdon]

Ja, das kommt davon, wenn man zwischendurch einiges auskommentiert, um es auch ohne GeoIP testen zu können

[lunar]

@think: Bis auf die fehlende Fehlerbehandlung, die derdon schon erwähnt hat, ist das Skript soweit gut.

@derdon: "parse_option()" ist irreführend, den die Argumente werden in dieser Funktion überhaupt nicht betrachtet. Entweder gibt die Funktion die geparsten Argumente zusammen mit dem Parser zurück, oder man nennt die Funktion "create_parser()" oder ähnlich.

"set_default_locale()" finde ich auch unschön. Die Funktion vermischt zwei grundverschiedene Dinge, nämlich die Initialisierung der Sprachumgebung sowie das Herausfinden der Sprache. Zudem ist sie irreführend benannt, denn ihr eigentlicher Zweck ist das Herausfinden der Sprache und des Landes, nicht das Initialisieren der Sprachumgebung.

"locale" zu übergeben, um die "testability" zu verbessern, ist meines Erachtens etwas übertrieben, tut die Funktion doch nicht mehr, als die "locale"-API zu nutzen. Darauf kann man sich auch ohne testen verlassen, und mithin zugunsten der Lesbarkeit auf die umständliche Übergabe verzichten. Ich halte es zudem für problematisch, Quelltext zu schreiben, der nur und ausschließlich zur Unterstützung der Tests dient. So testet man defacto Quelltext, der produktiv niemals ausgeführt wird, und müllt den Quelltext mit Logik zu, die für den eigentlichen Zweck des Programms vollkommen irrelevant ist. Ich halte es für sinnvoller, Tests und Quelltext strikt voneinander zu trennen, und nach Möglichkeit Quelltext zu testen, der in dieser Form auch wirklich im Produktivsystem ausgeführt wird. Testen kann man die Funktion ja trotzdem noch über Monkey-Patching in den entsprechenden Tests. Das ist in Python problemlos möglich und wird von jeder vernünftigen Testbibliothek unterstützt.

Wenn man die Funktion so implementieren möchte, dann besser als "get_default_language_and_territory/()" mit "locale.getdefaultlocale()", um den Seiteneffekt loszuwerden und den Zweck der Funktion zu verdeutlichen.

Ich würde die Funktion aber reduzieren, zu einem "parse_language_code(language_code)", aufgerufen in "main()" als "language, territory = parse_language_code(locale.getlocale()[0])" in "main()". Vorher ist natürlich noch ".setlocale()" nötig. Da diese Funktion den gesamten Prozess betrifft, gehört sie imho in die Eintrittsfunktion, und zwar zum frühestmöglichen Zeitpunkt, noch vor dem Parsen der Argumente.

So sind die Seiteneffekte nach Möglichkeit vom Rest des Programms getrennt, jede Funktion hat eine klare Aufgabe und ist unabhängig zu testen.

[think]

@think: Bis auf die fehlende Fehlerbehandlung, die derdon schon erwähnt hat, ist das Skript soweit gut.

Bis hierhin bin ich gut mitgekommen. *g*
-> Fehlerbehandlung ist klar, de_US ist selbstverständlich ungültig, wird aber momentan nicht abgefangen.

Den Rest muss ich mir mal in einer ruhigen Minute durchlesen. Ich denke zwar - mehr oder weniger - verstanden zu haben, was du meinst, bin mir aber nicht überall sicher.

Ich werde das Script anhand deiner Gedanken versuchen zu ändern und dann hier posten.