Funktion _innerhalb_ Klassenmethode schreiben?

[Hellstorm]

Hallo!

Ich schreibe gerade eine Klasse, bei der innerhalb einer Methode wieder eine Funktion aufgerufen werden soll. Da ich diese Funktion aber nur innerhalb dieser Methode brauche, stehe ich jetzt vor der Frage, ob ich die Funktion einfach innerhalb der Methode definieren soll.

Bei Python kann man ja eine Funktion in einer Funktion definieren (ist das eigentlich guter Stil?), aber ist das auch bei einer Klasse sinnvoll?

Folgendes Beispiel:

Code: Alles auswählen

class Test(object):
    def machwas(self):
         def saghallo():
             print("Hallo")
         saghallo()

Wäre das soweit gut? Oder sollte ich dieses saghallo() eher als weitere Klassenmethode einbauen und dann mit self.saghallo() aufrufen?

Danke!

[BlackJack]

@Hellstorm: Das kann man so allgemein nicht sagen, und bei dem sinnfreien Beispiel schon gar nicht, denn da würde ich schon in Frage stellen das `machwas()` eine ”Methode” ist, denn semantisch ist es keine.

Funktionen in Funktionen schreibe ich eigentlich nur wenn es sich um ein Closure handelt, man sie also nicht ausserhalb schreiben *kann*. Und wenn es tatsächlich eine *Funktion* ist, würde ich auch gut überlegen die in die Klasse zu schreiben, statt einfach wie bei Funktionen üblich auf Modulebene.

[Hellstorm]

Hm, in meinem Fall habe ich hier folgendes:

Code: Alles auswählen

class Upload(object):
    def __init__(self, server=None, files=[]):
        self.server = server

        if len(files) == 0:
            self.files = files
        else:
            self.files = [self.add_file_path(file) for file in files]

    def add_file_path(self, file_path):
        self.files.append({"file": open(file_path, "rb"),
                           "file_name": os.path.basename(file_path)})

    def start_upload(self):
        self.ssh_client = self._connect_to_ssh(self.server)
        self.sftp = self.ssh_client.open_sftp()

        for file_index, file_information in enumerate(self.files):
            self._upload(file_index)

    def _connect_to_ssh(self, server_data):
        ssh_client = paramiko.SSHClient()
        ssh_client.load_system_host_keys()
        ssh_client.connect(server_data["address"],
                    server_data["port"],
                    server_data["username"]
                    #server_data["password"],
                    #server_data["ssh_key"],
                    )
        return ssh_client

    def _upload(self, file_index):
        print("Uploading file {}...".format(self.files[file_index]["file_name"]))
        remote_file_name = os.path.join(self.server["remote_path"] + self.files[file_index]["file_name"])
        self.sftp.putfo(
                self.files[file_index]["file"],
                remote_file_name,
                self._show_progress)
        stdin, stdout, stderr = self.ssh_client.exec_command(
            "{} {}".format(self.server["remote_database_script"], remote_file_name))

    def _show_progress(self, bytes_transferred, total_bytes):
        pass

Mir geht es da jetzt um die Callback-Funktion um eine Fortschrittsanzeige anzuzeigen. Die würde ich ja nur in Upload._upload() gebrauchen wollen, so dass ich mir überlegt habe, ob ich sie auch direkt dort definieren könnte. Oder dann doch lieber Upload.__show_progress()?

[BlackJack]

@Hellstorm: Ähm, das ist aber ziemlich kaputt.

Das ``if``/``else`` in der `__init__()` macht keinen Sinn. Wenn das `files`-Argument eine leere Sequenz ist, dann kann man trotzdem die „list comprehension” (LC) ausführen lassen, die würde dann halt eine leere Liste erzeugen. Also mit einer simplen Zeile den selben Effekt.

Allerdings versucht der `add_file_path()`-Aufruf in der LC etwas an eine Liste anzuhängen die es noch gar nicht gibt als Attribut, weil das Ergebnis der LC, ja erst an dieses Attribut gebunden wird wenn sie fertig ausgewertet ist. Da die Methode `None` zurück gibt, wäre das ausserdem eine Liste mit lauter `None`-Werten, also ziemlich sinnfrei.

Das nächste Problem wäre dann das Dateihandles keine unendlich vorhandene Ressource sind. Einfach mal so eine unbestimmte Anzahl von Dateien auf ”Vorrat” öffnen ist eine ganz schlechte Idee. Diese gesamte ”Vorverarbeitung” verstehe ich auch nicht. Das kann man doch auch dann erledigen wenn es tatsächlich nötig wird.

Den Namen `file` würde ich auch nicht an etwas anderes als den eingebauten Datentyp `file` binden.

In `start_upload()`, was eigentlich nur `upload()` heissen sollte, denn es startet ja aus Sicht des Aufrufers nicht nur das Hochladen, sondern es ist danach auch komplett durchgeführt, werden plötzlich neue Attribute eingeführt. Ein Objekt sollte nach der `__init__()` vollständig initialisiert und benutzbar sein, und dort sollte man auch ablesen können was den Zustand des Objekts ausmacht. In dem Zusammenhang ist auch `None` als Default-Wert für `server` fragwürdig, denn ohne den kann man nichts hochladen.

`_connect_to_ssh()` ist semantisch so wie es dort steht keine Methoden. Da würde ich entweder eine Funktion oder eine *echte* Methode draus machen.

Die ganze Klasse ist IMHO komisch. Ich würde das erst mal als Funktionen schreiben und dann sehen ob sich da überhaupt irgend etwas *sinnvoll* zu einem eigenen Datentyp zusammenfassen lässt.

[Hellstorm]

@Hellstorm: Ähm, das ist aber ziemlich kaputt.

Das ``if``/``else`` in der `__init__()` macht keinen Sinn. Wenn das `files`-Argument eine leere Sequenz ist, dann kann man trotzdem die „list comprehension” (LC) ausführen lassen, die würde dann halt eine leere Liste erzeugen. Also mit einer simplen Zeile den selben Effekt.

Hm, ok, das wusste ich nicht, ich werds mal ausprobieren. Danke.

Allerdings versucht der `add_file_path()`-Aufruf in der LC etwas an eine Liste anzuhängen die es noch gar nicht gibt als Attribut, weil das Ergebnis der LC, ja erst an dieses Attribut gebunden wird wenn sie fertig ausgewertet ist. Da die Methode `None` zurück gibt, wäre das ausserdem eine Liste mit lauter `None`-Werten, also ziemlich sinnfrei.

Das nächste Problem wäre dann das Dateihandles keine unendlich vorhandene Ressource sind. Einfach mal so eine unbestimmte Anzahl von Dateien auf ”Vorrat” öffnen ist eine ganz schlechte Idee. Diese gesamte ”Vorverarbeitung” verstehe ich auch nicht. Das kann man doch auch dann erledigen wenn es tatsächlich nötig wird.

Ich finde das auch etwas unschön, aber da habe ich einen Grund für: Ich will dort später auch noch BytesIO-Objekte hochladen lassen (wenn mehrere Dateien z.B. vorher gezippt werden). Da Paramiko leider zwei Funktionen zum Hochladen anbietet (put und putfo), hätte ich dann das Problem, dass ich nachher umständlich prüfen müsste, was denn dort drin steht. Da sowieso keine 3000 Dateien hochgeladen werden würden, dachte ich, dass das ausreicht.

Die ganze Klasse ist IMHO komisch. Ich würde das erst mal als Funktionen schreiben und dann sehen ob sich da überhaupt irgend etwas *sinnvoll* zu einem eigenen Datentyp zusammenfassen lässt.

Ich hab das sogar erst als Funktion probiert, aber dann stand ich vor dem Problem, wie ich das denn mit der progressbar (https://code.google.com/p/python-progressbar/) lösen sollte:

Die put- (bzw. putfo)-Methode erwartet ja eine callback-Funktion mit den zwei Parametern „übertragene Bytes“ und „gesamte Bytes“. Aber wie soll ich in der Funktion denn dann auf das progressbar-Objekt verweisen, wenn man doch tunlichst keine global-Variablen verwenden darf? Normalerweise könnte ich der Funktion ja als dritten Parameter eben das progressbar-Objekt übergeben, aber da die Parameter doch hierbei vorgegeben sind, wüsste ich nicht, wie ich das sonst hätte machen sollen?


Den Rest werde ich mal korrigieren, danke!

[BlackJack]

@Hellstorm: Ich würde das öffnen der Dateien als Programmfehler ansehen. *Das* könnte man zum Beispiel in ein oder zwei Datentypen kapseln für Dateinamen und BytesIO-Objekte.

Und bei den Rückruffunktionen kann man entweder einen Datentyp schreiben der *das* kapselt, oder man schreibt ein Closure. Entweder tatsächlich als Funktion, oder man erstellt eine Funktion mit `functools.partial()`.

[BlackJack]

Ein völlig ungetesteter Entwurf:

Code: Alles auswählen

#!/usr/bin/env python
from __future__ import print_function
import os
import progressbar
from paramiko import SSHClient


class NullProgressBar(object):
    def __init__(self):
        pass

    def start(self):
        pass

    def update(self, current_value, max_value=None):
        pass

    def finish(self):
        pass


class ProgressBar(progressbar.ProgressBar):
    def __init__(self):
        progressbar.ProgressBar.__init__(self)

    def update(self, current_value, max_value=None):
        if max_value is not None:
            self.maxval = max_value
        progressbar.ProgressBar.update(self, current_value)


class File(object):
    def __init__(self, path):
        self.path = path

    @property
    def filename(self):
        return os.path.basename(self.path)

    def upload(self, sftp, remote_path, callback=None):
        remote_filename = os.path.join(remote_path, self.filename)
        sftp.put(self.path, remote_filename, callback)
        return remote_filename


class FileWrapper(object):
    def __init__(self, file_like, filename):
        self.file_like = file_like
        self.filename = filename
        
    def upload(self, sftp, remote_path, callback=None):
        remote_filename = os.path.join(remote_path, self.filename)
        self.file_like.seek(0, os.SEEK_END)
        file_size = self.file_like.tell()
        self.file_like.seek(0, os.SEEK_SET)
        sftp.putfo(self.file_like, remote_filename, file_size, callback)
        return remote_filename


class Server(object):
    def __init__(self, config, create_progress_bar=NullProgressBar):
        self.config = config
        self.create_progress_bar = create_progress_bar
        self.ssh_client = SSHClient()
        self.ssh_client.load_system_host_keys()
        self.ssh_client.connect(
            self.config['address'],
            self.config['port'],
            self.config['username'],
            #self.config['password'],
            #self.config['ssh_key'],
        )
        self.sftp = self.ssh_client.open_sftp()

    def __enter__(self):
        return self

    def __exit__(self, *_args):
        self.close()

    def upload(self, file_):
        print('Uploading file {0.filename}...'.format(file_))
        progress_bar = self.create_progress_bar()
        progress_bar.start()
        remote_filename = file_.upload(
            self.sftp, self.config['remote_path'], progress_bar.update
        )
        progress_bar.finish()
        self.ssh_client.exec_command(
            '{0} {1}'.format(
                self.config['remote_database_script'], remote_filename
            )
        )

    def upload_many(self, files):
        for file_ in files:
            self.upload(file_)

    def close(self):
        self.sftp.close()
        self.ssh_client.close()


def main():
    config = ...
    file_like = ...
    files = [File('/path/to/file'), FileWrapper(file_like, 'filename')]
    with Server(config, ProgressBar) as server:
        server.upload_many(files)

[Hellstorm]

Hallo,

danke! Ich schau mir das mal an. Sieht ja schon mal recht komplex aus

Ich habe in der Zwischenzeit auch mal etwas ausprobiert, völlig ohne Klassen:

Code: Alles auswählen

def connect_to_ssh(server_data):
    ssh_client = paramiko.SSHClient()
    ssh_client.load_system_host_keys()
    #TODO: Error abfangen wenn nicht funktioniert.
    ssh_client.connect(server_data["address"],
                server_data["port"],
                server_data["username"]
                #server_data["password"],
                #server_data["ssh_key"],
                )
    return ssh_client

def create_file_path_dict(file_path):
    return {"file_entry" : file_path,
            "file_size" : os.path.getsize(file_path),
            "file_name" : os.path.basename(file_path)}

def upload(file_entry, server_data, sftp_object):
    def callback(bytes_transferred, bytes_total):
        progress.update(bytes_transferred)

    remote_file_name = os.path.join(server_data["remote_path"] + file_entry["file_name"])

    print("Lade Datei {} hoch.".format(file_entry["file_name"]))
    try:
        progress = progressbar.ProgressBar(maxval=file_entry["file_size"]).start()
        sftp_object.put(file_entry["file_entry"],
                        remote_file_name,
                        callback)
        progress.finish()
        print("Erfolgreich hochgeladen")
    except TypeError as e:
        progress = progressbar.ProgressBar(maxval=file_entry["file_size"]).start()
        sftp_object.putfo(file_entry["file_entry"],
                          remote_file_name,
                          file_entry["file_size"],
                          callback)
        progress.finish()
        print("Erfolgreich hochgeladen")

def main():
    #TODO: Auf Argparse umstellen.
    files = [create_file_path_dict(file_entry) for file_entry in sys.argv[1:]]

    ssh_client = connect_to_ssh(server)
    sftp = ssh_client.open_sftp()
    for entry in files:
        upload(entry, server, sftp)
    sftp.close()
    ssh_client.close()