Pylint: "* or ** magic"

Wenn du dir nicht sicher bist, in welchem der anderen Foren du die Frage stellen sollst, dann bist du hier im Forum für allgemeine Fragen sicher richtig.
Antworten
Benutzeravatar
str1442
User
Beiträge: 520
Registriert: Samstag 31. Mai 2008, 21:13

Mittwoch 22. Oktober 2008, 14:45

C:191:register_archetype._archetyped: Missing docstring
W:192:register_archetype._archetyped: Used * or ** magic
C:205:register_type._typed: Missing docstring
W:206:register_type._typed: Used * or ** magic
C:219:register_object._objected: Missing docstring
W:220:register_object._objected: Used * or ** magic
W:277:init_archetype: Used * or ** magic
W:369:init_object: Used * or ** magic
Jemand ne Ahnung warum das als "magic" und Warning gewertet wird?

Geht um einen Aufruf der Art:

Code: Alles auswählen

def f(*args, **kwargs):
    pass
Und einige andere Entpack Arten in der Weise.
BlackJack

Mittwoch 22. Oktober 2008, 16:00

Ich denke Ned's Begründung zielt zu sehr auf auf `pylint` selbst ab. Nicht nur `pylint` hat bei so einer Signatur das Problem nicht zu wissen was die Funktion denn nun eigentlich erwartet, sondern Menschen haben das auch.

Funktionen die eine beliebige Anzahl von Argumenten und Schlüsselwortargumenten entgegen nehmen, sind IMHO magisch. Das ist etwas besonderes und kommt im Grunde auch recht selten vor.
Benutzeravatar
str1442
User
Beiträge: 520
Registriert: Samstag 31. Mai 2008, 21:13

Mittwoch 22. Oktober 2008, 16:23

BlackJack:

Klar, eine Funktion die so agiert ist schwer einzuschätzen und auch meist eher unnützlich, weswegen ein solchen Beispiel wie def f(*args, **kwargs) eher für Funktionen genutzt werden sollte, die die Argumente nur weitergeben, aber das ist ja nicht der einzige Einsatzzweck für * und **.

Ich benutze es in dem Code, den pylint oben analyisiert hat, einmal dazu, um ein bereits fertig sortiertes dict entpackt an eine Funktion zu übergeben. Da sieht man beim Betrachten des Codes sofort, das das Dict alle Parameter der Funktion ausfüllt. Ein anderes Beispiel wäre dieses:

Code: Alles auswählen

for name in names:
    # Do something
    bla(**{name: argument})
Das ist auch etwas magisch, aber der einzige Weg um ein bestimmtes Keyword Argument dynamisch an eine Funktion zu übergeben, und verwirrt wird man, hat man das Prinzip verstanden, auch nicht sonderlich davon. Aber auch bei sowas schlägt pylint Alarm, und das ohne genauere Angaben. Es sei eben magisch, und Punkt.
BlackJack

Mittwoch 22. Oktober 2008, 16:30

Bei dem Beispiel meckert `pylint` IMHO zu recht. Wieso müssen das Schlüsselwortargumente sein? Wie sieht die Signatur von `blah()` aus? Das ist IMHO sehr merkwürdig und ein "code smell".
Benutzeravatar
str1442
User
Beiträge: 520
Registriert: Samstag 31. Mai 2008, 21:13

Mittwoch 22. Oktober 2008, 16:42

Meinst du das zweite Beispiel? Der genaue Code ist der hier:

Code: Alles auswählen

    object_generated_cls = init_object(filename, kwargs[NAME])

    del kwargs[NAME]

    # Get a partial object and delete the main branch. See init_object().

    for section in kwargs.keys()[:]:
        
        try:
            branch = dict(izip((pkg.NAME for pkg in BRANCHES),
                               BRANCHES))[section]
        except KeyError:
            raise ValueError("%s: Given Branch doesnt exist: %s" %
                             (filename, section))

        # Get branch by name equality of section and pkg.NAME

        object_generated_cls = partial(object_generated_cls,
                                       **{section: branch.init_object(filename,
                                                          kwargs[section])})

        # Generate a partial "stack" - for every branch execute a partial
        # on the last partial object, with the branch name: branch.init_object()
        # as a keyword argument. This way, we "nest" some partial functions,
        # saving every keyword argument in the stack. The result is a
        # partial object we just have to execute to get a new object from
        # from the archetype.

        del kwargs[section]
Es geht um das Einlesen einer Konfigurationsdatei mittels ConfigParser. Diese hat verschiedene Sektionen, die sich auf Module beziehen, die ich in dem Tuple BRANCHES gespeichert habe. Jedes Modul hat seinen Namen, es wird dann für jede Section das entsprechende Modul rausgesucht. Danach wird mit Hilfe mehrerer partial Aufrufe das Objekt immer um den Rückgabewert der entsprechenden init_object() funktionen an das Objekt drangehängt.


Was ist ein "Code Smell"?

Dennoch, sagt pylint ja scheinbar nichts direkt dagegen, sondern nur, weil ich eben * und ** anwende.
BlackJack

Mittwoch 22. Oktober 2008, 17:04

Äh, okay, ich verstehe nicht was da passiert. Damit ist das schonmal schlechter Code. ;-)

Wozu soll dieser "Stack" gut sein? Kann man nicht *ein* Dictionary erstellen und dann *einmal* `partial()` aufrufen? Und gibt es irgendwo eine Funktionssignatur die so ähnlich aussieht ``f(section_a, section_b, section_c)``, eben mit den Section-Namen? Oder läuft das darauf hinaus, dass da eine ``f(**sections)`` existiert? In dem Fall stellt sich die Frage warum die Funktion "Magie" benutzt und nicht einfach ein Dictionary entgegen nimmt, dann kann man sich die Magie beim Aufruf auch sparen.

Ein "code smell" ist Code der verdächtig nach einem Entwurfsfehler aussieht. Zum Beispiel weil er zu magisch oder zu komplex aussieht.
Benutzeravatar
str1442
User
Beiträge: 520
Registriert: Samstag 31. Mai 2008, 21:13

Mittwoch 22. Oktober 2008, 17:26

Und gibt es irgendwo eine Funktionssignatur die so ähnlich aussieht ``f(section_a, section_b, section_c)``, eben mit den Section-Namen?
Ja, exakt. Ich hab zum Beispiel dieses Konfigurationsdatei:
# Missile.archetype

[main]
base: BaseMissile
main_hull: 5

[physics]
base: PhysicsObject
mass: 10
max_acceleration: 5

[ai]
base: MissileAi

[render]
base: MissileRender
model: Missile.png
size: [5 5]
In der "base" Variable wird angegeben, welches Objekt jeweils gemeint ist. Jede Sektion hat ihre eigenen Objekte, die dort definiert werden. "main" steht für das "Hauptobjekt". In der Funktion wird mithilfe von partial nun für jede Sektion das entsprechende Objekt herausgesucht und Stück für Stück zusammengebaut. Das Hauptobjekt hat die Struktur:

Code: Alles auswählen

class MissileObject:
    def __init__(self, main_hull, physics, ai, render)
    ....
Das Endergebnis ist ein Objekt, das ich nur aufzurufen brauche, um eine neue, bereits fertig konfigurierte Instanz des Objektes zu erhalten. Physics, Render und Ai sind die Objekte, die dann die Repräsentation des Objekts in ihrer sektion darstellen. So ist jeder Teil vom anderen getrennt und kann für sich ausmachen, wie er mit was umgeht.
BlackJack

Mittwoch 22. Oktober 2008, 17:56

Bleibt die Frage mit dem "Stack", ist der wirklich nötig? IMHO zu komplex.

Was ist mit dem löschen von ``kwargs[section]``? Ist das notwendig? Erwartest Du dass in `kwargs` am Ende etwas übrig bleibt oder ist dass dann grundsätzlich leer?
Benutzeravatar
str1442
User
Beiträge: 520
Registriert: Samstag 31. Mai 2008, 21:13

Mittwoch 22. Oktober 2008, 18:11

Naja, der Code ist in der Form, wie er ist, sehr symmetrisch und besitzt keinerlei Ausnahmen, die man beachten müsste. Aber ich bin grade dabei, das nochmal zu überprüfen. Theoretisch ginge es wohl schon, nur weiß ich nicht, ob dann die einzelnen init_object Methoden (und daraus folgendend der Aufbau der Sektionsmodule) stärker voneinander abweichen, insbesondere in den Rückgabewerten. Ich versuch den Umbau grade. (Dank Git ;) )

kwargs sollte am Ende leer sein, ja. Es geht ja (bei den sogenannten "archetypes") nur darum, die einzelnen Objekte in einen Ausführbereiten Zustand zu bringen und dann ein großes partial-objekt draus zu generieren. Etwas anderes als Sektionen und deren Objekte werden also per se nicht behandelt. Wenn es nicht leer ist, gäbe es also auch nichts, was logisch gesehen passend wäre, noch abgearbeitet zu werden.
lunar

Mittwoch 22. Oktober 2008, 18:38

BlackJack hat geschrieben:Funktionen die eine beliebige Anzahl von Argumenten und Schlüsselwortargumenten entgegen nehmen, sind IMHO magisch. Das ist etwas besonderes und kommt im Grunde auch recht selten vor.
Das halte ich für ein Gerücht. "dict()" und "dict.update()" sind nicht selten, und würden ohne ** nicht funktionieren.

Ob das magisch ist, hängt von der Funktion ab. Ich stimme dir insoweit zu, dass es schlecht ist, * oder ** zu verwenden, weil man zu faul ist, eine größeren Anzahl von Argumenten Namen zu geben. Dann ist refactoring angebracht, weil die Funktion zu viele Argumente entgegen nimmt.

Immer dann allerdings, wenn eine Funktion eine Liste oder ein Wörterbuch entgegennimmt, ist die Verwendung dieser Syntax eine Überlegung wert. Ist absehbar, dass diese Funktion Objekten aufgerufen wird, die lokal erzeugt werden, ist die Sternchensyntax imho zu bevorzugen.

Imho ist es nämlich in so einem Fall wesentlich hässlicher, ein explizites Wörterbuch zu verwenden:

Code: Alles auswählen

configfile.update({'myoption': 'foo', 'anotheroption': 'spam'})
# oder
configfile.update(myoption='foo', anotheroption='spam')
Ich persönlich halte die zweite Variante für wesentlich eleganter.

Etwas anderes ist es natürlich, wenn das Wörterbuch in den meisten Fällen direkt übergeben wird. Aber diese Fälle kann man mit geschickter Signatur eigentlich recht elegant lösen.
Benutzeravatar
str1442
User
Beiträge: 520
Registriert: Samstag 31. Mai 2008, 21:13

Mittwoch 22. Oktober 2008, 18:45

Okay, abgeändert sieht die komplette Funktion jetzt so aus:

Code: Alles auswählen

@register_archetype
def init_archetype(filename_csv_name, name_csv_name, description_csv_name,
                  kwargs):
    """init_archetype: Generate new archetype deliver function.
    Return dict archetype_name: {INTERNAL_DESCRIPTION_NAME: <description>,
                                 INTERNAL_OBJECT_CLS_NAME: <archetype deliver
                                                            function>}

    Parameters: filename_csv_name - the name of the filename variable in the
                                    archetype csv file.
                name_csv_name - the name of the archetypename variable in the
                                archetype csv file.
                description_csv_name - the name of the description variable
                                       in the archetype csv file.
                kwargs - the archetype dict read in the control package by
                         the parse module.

    """
    filename = kwargs[filename_csv_name]
    name = kwargs[name_csv_name]
    description = kwargs[description_csv_name]
    basetype = kwargs[NAME][RESOURCE_BASE_NAME]

    del (kwargs[filename_csv_name], kwargs[name_csv_name],
         kwargs[description_csv_name], kwargs[NAME][RESOURCE_BASE_NAME])
    
    # Get the variables from the top level and the basetype we are using,
    # then delete everything toplevel and not section related and basetype
    # reference

    try:
        object_cls = dict(izip((cls.__name__ for cls in BASETYPES),
                            BASETYPES))[basetype]
    except KeyError:
        raise ValueError("%s: Given Configuration contains "
                         "an illegal type: %s" % (filename, basetype))

    # Get object class from the basetype name

    object_generated_dict = {}
    object_generated_dict.update(kwargs[NAME])

    # Set new dict in which we are saving then newly generated persistent
    # call functions and options for the object_cls

    del kwargs[NAME]

    # Delete main section so we dont iterate over it

    for section in kwargs:
        
        try:
            branch = dict(izip((pkg.NAME for pkg in BRANCHES),
                               BRANCHES))[section]
        except KeyError:
            raise ValueError("%s: Given Branch doesnt exist: %s" %
                             (filename, section))

        # Get branch by name equality of section and pkg.NAME

        object_generated_dict[section] = branch.init_object(filename,
                                                            kwargs[section])

        # Set the newly generated section object to the section name

    return {name: {INTERNAL_DESCRIPTION_NAME: description,
                   INTERNAL_OBJECT_CLS_NAME: partial(object_cls,
                                                     **object_generated_dict)}}
Und funktioniert. Ist auch definitiv die bessere Lösung, hier brauch ich eine init_object Funktion für die main section überhaupt nicht, war schon ein wenig paradox eine solche Funktionen zu haben, wenn die Abstrahierung gar nicht notwendig ist (Da ich nicht auf ein anderes Modul zugreifen muss).

Aber um mal zum eigentlichen Thema zurückzukommen:
W:299:init_archetype: Used * or ** magic
pylint beschwert sich weiterhin nichtssagend, und hier ist ** (in der letzten Zeile) ja so ziemlich im Sinne der Erfindung angewandt.
BlackJack

Mittwoch 22. Oktober 2008, 19:13

Natürlich warnt `pylint` weiterhin. Da wird ja immer noch Magie verwendet. Zwar "bestimmungsgemäss", aber wenn `pylint` *das* erkennen könnte, sind wir wahrscheinlich so weit, dass man keine Programme mehr schreiben braucht, weil der Computer das selber macht.

Dir ist klar, dass das eine *Warnung* ist, und kein Fehler der unbedingt behoben werden muss? Man sollte halt bei der */**-Magie immer nachschauen ob man das nicht anders, weniger magisch lösen kann. Wenn's nicht anders einfacher geht, dann geht's halt nicht. In dem Fall würde ich die betreffende Stelle per Kommentar von `pylint`\s Prüfung ausnehmen, dann hat man auch gleich dokumentiert, dass man darüber nachgedacht hat, und das wirklich so haben möchte.
Benutzeravatar
str1442
User
Beiträge: 520
Registriert: Samstag 31. Mai 2008, 21:13

Mittwoch 22. Oktober 2008, 19:24

Natürlich warnt `pylint` weiterhin. Da wird ja immer noch Magie verwendet. Zwar "bestimmungsgemäss", aber wenn `pylint` *das* erkennen könnte, sind wir wahrscheinlich so weit, dass man keine Programme mehr schreiben braucht, weil der Computer das selber macht.
Nur, das ich diese Magie für eben keine halte. Ich kann natürlich */** irgendwie zweckentfremden, das geht mit fast allem. Das pylint aber generell davor warnt, kam mir eben komisch vor, weswegen ich den Thread ja gen Anfang eröffnete. Hätte ja sein können, das py3k irgendwo an der Geschichte rumschraubt, und google:* ** python funktioniert nicht so gut ;) Und, wie Lunar schon sagte, ist * und ** auch nicht der geheime Superjoker. Im Gegenteil, ich empfinde Code mit * oder ** sogar meist als sehr elegant, wenn es "richtig" genutzt wird, und verwende es selber sehr gerne.

Oder vielleicht hätte pylint ja auch nur bei Funktionsdefinitionen mit * oder ** gewarnt, da eben dort zb die Gefahr der Verwendung besteht, die Lunar aufgezeigt hat. Was eben nicht so ist.
Leonidas
Administrator
Beiträge: 16024
Registriert: Freitag 20. Juni 2003, 16:30
Kontaktdaten:

Mittwoch 22. Oktober 2008, 19:54

lunar hat geschrieben:
BlackJack hat geschrieben:Funktionen die eine beliebige Anzahl von Argumenten und Schlüsselwortargumenten entgegen nehmen, sind IMHO magisch. Das ist etwas besonderes und kommt im Grunde auch recht selten vor.
Das halte ich für ein Gerücht. "dict()" und "dict.update()" sind nicht selten, und würden ohne ** nicht funktionieren.
Ja, ich halte die Sternchen auch für praktisch. Es ist auch nützlich Code kompatibel zu halten. Ich hatte den Fall, dass ich irgendeine Django-Klasse erweitert habe und dort die übergebenen args und kwargs in meiner Methode abgefangen habe, dort meine eigenen Parameter rausgesammelt habe und den Rest an die Methode in der Elternklasse weitergeleitet habe.
My god, it's full of CARs! | Leonidasvoice vs Modvoice
Antworten