Peer Review von Richards Gewurschtel

Begonnen von RichardCZ, 31 März 2020, 22:17:53

Vorheriges Thema - Nächstes Thema

RichardCZ

Zitat von: KernSani am 06 April 2020, 07:53:09
98_DSBMobile hatte offensichtlich kein Problem mit Perltidy (verwende ich ohnehin immer) aber falsches encoding. Hab das mal geändert und eingecheckt- macht sich das bei dir jetzt irgendwie bemerkbar?


Gesendet von iPhone mit Tapatalk

Sieht nicht so aus, das Encoding ist ja bereits teil der Normalisierung:

https://gl.petatech.eu/root/HomeBot/-/commit/29d9e22953d6ece9d3d718290d022738afb7fbd1

ein paar andee diffs sind wohl drin. Für mich sieht auch das funktional aus.
Witty House Infrastructure Processor (WHIP) is a modern and
comprehensive full-stack smart home framework for the 21st century.

RichardCZ

#31
Zitat von: Wzut am 06 April 2020, 07:29:14
Ich würde dir ja einen Keks geben wenn sich sehen könnte was die Waschstrasse abgewaschen hat.
Was ich da sehe sind lediglich die Diffs zum Vorgänger ? Gerade das Thema UTF8 hätte mich interessiert ob da nun alles stimmt,

Was die Waschstraße abwäscht sieht man in den riesigen diffs von gestern. Ab dem Zeitpunkt waren im Hobo die normalisierten Codes die Basis und ab dann - da ja jeder neue diff vom SVN auch durch die Normalisierung gejagt wird - sieht man eben nur noch den diff den man auch im SVN sieht ... nur eben in einem perltidy-formatierten code.

vgl.

https://svn.fhem.de/trac/changeset?reponame=&new=21606%40%2F&old=21605%40%2F
und
https://gl.petatech.eu/root/HomeBot/-/commit/295596c9328eecb738970a366350019d11987331?view=inline#caf2fc0b293b32e58dd0a79f5e73513fec123413_1_1

("view inline" - vmtl. besser, weil es im FHEM trac per default auch so angezeigt wird.)
Witty House Infrastructure Processor (WHIP) is a modern and
comprehensive full-stack smart home framework for the 21st century.

RichardCZ

Zitat von: RichardCZ am 04 April 2020, 20:36:51
künftig - so dachte ich:

sub XXX_Initialize {

    return {
        DefFn   => 'XXX_Define', # besser: coderef
        UndefFn => 'XXX_UNdef',
...
    };
}


Man bekommt es schon ein wenig auch so hin:

my %hash = (test => 1);

Moerder_Init(\%hash);  # oder halt eben &{$initfn}(\%hash) siehe CommandReload

sub Moerder_Init {
    my $hash = shift;

    %{$hash} = (
        %{$hash},
        DefFn   => 'Knödel',
        UndefFn => 'Dödel',
        SetFn   => 'Moe',
        GetFn   => 'Larry',
        ReadFn  => 'Curly',
    );
}


Dieses %{$hash} im hash ist nicht notwendig, wenn man weiß, dass an Init kein vorbelegtes hash übergeben wurde (was - soweit ich das überblicken kann - auch nicht der Fall ist) ansonsten will man eventuell obiges test => 1 nicht verlieren. Also zumeist einfach kurz:

sub Moerder_Init {
    my $hash = shift;

    %{$hash} = (
        DefFn   => 'Knödel',
        UndefFn => 'Dödel',
        SetFn   => 'Moe',
        GetFn   => 'Larry',
        ReadFn  => 'Curly',
    );
}


Man spart sich also die ganzen $hash->{xxx} = yyy; Hash lookups/settings pro Zeile und macht das in einem Aufwasch.
Witty House Infrastructure Processor (WHIP) is a modern and
comprehensive full-stack smart home framework for the 21st century.

Wzut

Zitat von: RichardCZ am 06 April 2020, 12:01:09
Man spart sich also die ganzen $hash->{xxx} = yyy; Hash lookups/settings pro Zeile
Nun gut , aber an der Stelle sehe ich den Nutzen nicht.
Neue FHEM Module sind für mich wie mein alter Lego Baukasten, (copy & paste) und ob der erste 8er Stein nun weiß oder rot ist, ich klick ihn einfach auf die Grundplatte, Hauptsache er sitzt fest und nicht locker :)   
Maintainer der Module: MAX, MPD, UbiquitiMP, UbiquitiOut, SIP, BEOK, readingsWatcher

RichardCZ

#34
Zitat von: Wzut am 06 April 2020, 13:24:30
Nun gut , aber an der Stelle sehe ich den Nutzen nicht.

Du sollst ja auch nicht Deine Termine beim Optiker schwänzen.  ;)

Altkanzler Schmidt hat zwar mal gesagt, wer Visionen hat soll zum Arzt gehen, aber ich
denke, ein wenig strategische Voraussicht kann nicht schaden.

sub Moerder_Init_jetzt {
    my $hash = shift;

    $hash->{DefFn} = "Knödel";
    $hash->{UndefFn} = "Dödel";
    $hash->{SetFn} = "Moe";
    $hash->{GetFn} = "Larry";
    $hash->{ReadFn} = "Curly";

}

sub Moerder_Init_dann {
    my $hash = shift;

    %{$hash} = (
        DefFn   => 'Knödel',
        UndefFn => 'Dödel',
        SetFn   => 'Moe',
        GetFn   => 'Larry',
        ReadFn  => 'Curly',
    );
}

sub Moerder_Init_noch_spaeter {
    return {
        DefFn   => 'Knödel',
        UndefFn => 'Dödel',
        SetFn   => 'Moe',
        GetFn   => 'Larry',
        ReadFn  => 'Curly',
    };
}

sub Moerder_Init_noch_spaeter_eventuell {
    return {
        DefFn   => \&Knoedel,
        UndefFn => \&Doedel',
        SetFn   => \&Moe,
        GetFn   => \&Larry,
        ReadFn  => \&Curly,
    };
}



Zitat von: Wzut am 06 April 2020, 13:24:30
Neue FHEM Module sind für mich wie mein alter Lego Baukasten, (copy & paste)

...und genau deswegen hat FHEM 1 mio Zeilen redundanten Code, dessen Funktionalität man auch in 100k Zeilen hinbekommen würde.
Witty House Infrastructure Processor (WHIP) is a modern and
comprehensive full-stack smart home framework for the 21st century.

CoolTux

Zitat von: RichardCZ am 06 April 2020, 13:46:55
Altkanzler Schmidt hat zwar mal gesagt, wer Visionen hat soll zum Arzt gehen, ...

Wer hat uns verraten ...   ;D
Du musst nicht wissen wie es geht! Du musst nur wissen wo es steht, wie es geht.
Support me to buy new test hardware for development: https://www.paypal.com/paypalme/MOldenburg
My FHEM Git: https://git.cooltux.net/FHEM/
Das TuxNet Wiki:
https://www.cooltux.net

RichardCZ

Ab und zu schaue ich zurück was mit dem Code "nach einer Reihe marginaler Codeänderungen" passiert ist:

fhem.pl

sub
IsDevice($;$)
{
  my $devname = shift;
  my $devtype = shift;

  return 1
    if ( defined($devname)
      && defined( $defs{$devname} )
      && (!$devtype || $devtype eq "" ) );

  return 1
    if ( defined($devname)
      && defined( $defs{$devname} )
      && defined( $defs{$devname}{TYPE} )
      && $defs{$devname}{TYPE} =~ m/^$devtype$/ );

  return 0;
}


HomeBot::Device modul

sub IsDevice :Export {
    my $devname = shift // return 0;
    my $devtype = shift || return 1;

    my $dev_hr = $main::defs{$devname} // return 0;
    my $type   = $dev_hr->{TYPE}       // return 0;

    return 1 if ($type =~ m{\A$devtype\z}xms);
    return 0;
}


Von heute auf morgen ging das nicht. Da sind 4-5 Schritte dazwischen - und der neuere Code hat noch bestimmt 2-3 vor sich.
Witty House Infrastructure Processor (WHIP) is a modern and
comprehensive full-stack smart home framework for the 21st century.

RichardCZ

fhem.pl

sub
setKeyValue($$)
{
  my ($key,$value) = @_;
  my $fName = AttrVal("global", "keyFileName", "uniqueID");
  $fName =~ s/\.\.//g;
  $fName = $attr{global}{modpath}."/FHEM/FhemUtils/$fName";
  my ($err, @old) = FileRead($fName);
  my @new;
  if($err) {
    push(@new, "# This file is auto generated.",
               "# Please do not modify, move or delete it.",
               "");
    @old = ();
  }
 
  my $fnd;
  foreach my $l (@old) {
    if($l =~ m/^$key:/) {
      $fnd = 1;
      push @new, "$key:$value" if(defined($value));
    } else {
      push @new, $l;
    }
  }
  push @new, "$key:$value" if(!$fnd && defined($value));

  return FileWrite($fName, @new);
}


hobo.pl (nur der teil ab FileRead)

sub setKeyValue {
    my $key   = shift;
    my $value = shift;

    my $fName = AttrVal(qw(global keyFileName uniqueID));

    $fName =~ s/\.\.//g;
    $fName = $attr{global}->{modpath}."/FHEM/FhemUtils/$fName";

    my ($err, @old) = FileRead($fName);

    my @new = !$err ? grep { ! m{^$key:}xms } @old            # if no error, pass non-matching lines 1:1
                    : ("# This file is auto generated.",      # else create new preamble
                       "# Please do not modify, move or delete it.",
                       '');

    push @new, "$key:$value" if (defined $value);             # if value defined, add that k:v as new

    return FileWrite($fName, @new);                           # store (overwrite) file with new values
}


Vermutlich kann man das noch zusammenschnurren. In-place regexp-replace.
Witty House Infrastructure Processor (WHIP) is a modern and
comprehensive full-stack smart home framework for the 21st century.

rudolfkoenig

Sehe ich richtig, dass der xms Regexp modifier an dieser Stelle ueberfluessig ist, oder uebersehe ich etwas?

RichardCZ

Zitat von: rudolfkoenig am 07 April 2020, 11:18:03
Sehe ich richtig, dass der xms Regexp modifier an dieser Stelle ueberfluessig ist, oder uebersehe ich etwas?

PBP 236-241. Es gibt einen durchaus nichtsubtilen Unterschied zwischen "überflüssig" und "nicht zwingend notwendig".

my %blah = (
    tralala => 1,
    sowieso => 1,
);


Da ist das Komma hinter sowieso => 1 auch "nicht zwingend notwendig". Jemand würde vielleicht von überflüssig sprechen. Ich nicht.
(ok, JSON-Geschädigte mögen das anders sehen)

m{ ^$key: }xms

Wer sich angewöhnt hat JEDE regex mit m{...}xms aufzusetzen, wird mit der Zeit u.A. ein feineres Gespühr für den Unterschied zwischen \A und ^ sowie \z und $ entwickeln, seine Regexen besser formatieren usw. Mittel-/langfristig fährt man so besser. IMHO.
Witty House Infrastructure Processor (WHIP) is a modern and
comprehensive full-stack smart home framework for the 21st century.

rudolfkoenig

Auf der anderen Seite muss man Selbstbewusst genug sein, um es besser zu wissen als die perl-Voreinstellung, Anfaenger haben auch schwerer, weil man xms _zusaetzlich_ verstehen muss. Ich will damit deine Argumente nicht herunterspielen, will nur sagen, dass auch andere Sichtweisen gibt.

RichardCZ

Zitat von: rudolfkoenig am 07 April 2020, 11:46:32
Auf der anderen Seite muss man Selbstbewusst genug sein, um es besser zu wissen als die perl-Voreinstellung, Anfaenger haben auch schwerer, weil man xms _zusaetzlich_ verstehen muss. Ich will damit deine Argumente nicht herunterspielen, will nur sagen, dass auch andere Sichtweisen gibt.

Andere Sichtweisen? Unvorstellbares Konzept.  ;)

Bei perlcritic -3 fängt er an /x anzumeckern
bei -2 kommt /m und /s dazu

wenn man's gleich macht ist man das schonmal los.

Ok - Anfänger mal außen vor gelassen (wobei ich ja glaube, wenn man denen sagt "mach m{...}xms", dann machen die das einfach cargo-kult-mäßig"): sehen wir uns das aus der Profi & Semi-Profi Perspektive an:

Ich wollte noch etwas über split schreiben, kann es aber auch gleich hier loswerden.

Das erste Argument von split ist immer eine Regex. Nun wird split in FHEM so häufig als split 'blah', $string verwendet, dass ich schon fast glaube, dass die meisten gar nicht wissen, dass 'blah' eine Regex ist. Da täte IMHO ein split m{blah}, $string schon viel um das Bewusstsein dafür zu schärfen.

Aber dann gibt es ja auch die Semiprofis:

@a = split("[ \t][ \t]*", $def, 3);

hier ahnt man wohl dass das eine Regex ist, und man möchte wohl auch "Leerzeichen und Tabs" abfangen. Der Profi würde vorschlagen:

@name = split m{ \s+ }xms, $def, 3;

ist jetzt nicht viel länger, IMHO lesbarer, intuitiver und vor allem matcht auch non-breakable spaces (im Idealfall diesen ganzen Sotter: http://jkorpela.fi/chars/spaces.html). Das wären derzeit 403 Stellen, wo man das ein für alle mal verbessern könnte.


$ grep -Fr '[ \t][' * | wc -l
403


Will sagen: Die drei modifier Zeichen machen das Kraut nicht fett, sie erlauben aber wesentlich lesbarere und korrekte(re) Regexps
Witty House Infrastructure Processor (WHIP) is a modern and
comprehensive full-stack smart home framework for the 21st century.

RichardCZ

Das funktioniert soweit alles gut - neuester Zugang in beiden Repos:
https://gl.petatech.eu/root/HomeBot/-/commit/4baccd217b3b336891d7a41c5183573cee098016

Ich werde den autosync jetzt mit Absicht für ne Weile abschalten, weil ich sehen möchte wie er dann damit zurechtkommt, wenn sich im FHEM mal mehr diffs angesammelt haben.
Witty House Infrastructure Processor (WHIP) is a modern and
comprehensive full-stack smart home framework for the 21st century.

RichardCZ

Da fing alles an:

sub
GetDefAndAttr($;$)
{
  my ($d, $dumpFUUID) = @_;
  my @ret;

  if($d ne "global") {
    my $def = $defs{$d}{DEF};
    if(defined($def)) {
      $def =~ s/;/;;/g;
      $def =~ s/\n/\\\n/g;
      push @ret,"define $d $defs{$d}{TYPE} $def";
    } else {
      push @ret,"define $d $defs{$d}{TYPE}";
    }
  }

  push @ret, "setuuid $d $defs{$d}{FUUID}"
        if($dumpFUUID && defined($defs{$d}{FUUID}) && $defs{$d}{FUUID});

# exclude attributes, format <deviceName>:<attrName>, space separated list
  my @dontSave = qw(configdb:rescue configdb:nostate configdb:loadversion
                    global:configfile global:version);
  foreach my $a (sort {
                   return -1 if($a eq "userattr"); # userattr must be first
                   return  1 if($b eq "userattr");
                   return $a cmp $b;
                 } keys %{$attr{$d}}) {
    next if (grep { $_ eq "$d:$a" } @dontSave);
    my $val = $attr{$d}{$a};
    $val =~ s/;/;;/g;
    $val =~ s/\n/\\\n/g;
    push @ret,"attr $d $a $val";
  }
  return @ret;
}


Mittlerweile bin ich da:

sub GetDefAndAttr {
    my $d         = shift;
    my $dumpFUUID = shift;

    my @ret;

    if ($d ne 'global') {
        my $def = $defs{$d}->{DEF} // '';

        $def =~ s/;/;;/g;
        $def =~ s/\n/\\\n/g;
        push @ret, "define $d $defs{$d}->{TYPE} $def";
    }

    push @ret, "setuuid $d $defs{$d}->{FUUID}"
        if ($dumpFUUID && defined($defs{$d}->{FUUID}) && $defs{$d}->{FUUID});

    # exclude attributes, format <deviceName>:<attrName>, space separated list
    my @dontSave = qw(configdb:rescue configdb:nostate configdb:loadversion
                    global:configfile global:version);

    for my $a (sort {
        return -1 if ($a eq "userattr"); # userattr must be first
        return  1 if ($b eq "userattr");
        return $a cmp $b;
    } keys %{$attr{$d}}) {
        next if (grep { $_ eq "$d:$a" } @dontSave);
        my $val = $attr{$d}->{$a};
        $val =~ s/;/;;/g;
        $val =~ s/\n/\\\n/g;
        push @ret,"attr $d $a $val";
    }

    return @ret;
}


und würde mich gerne dem zweiten Teil der sub widmen. Erstmal tief durchatmen. Also dass man nicht magische Variablen wie $a verwenden sollte ist bekannt. Dass for-Schleifenvariablen Referenzen auf die iterierte Liste bilden auch. Was zum Geier geschieht da? Welches $a ist welches?

https://de.wikipedia.org/wiki/Obfuscated_Perl_Contest

Wir sollten den vielleicht wieder zum Leben erwecken?

Also, wenn ich das recht verstehe, will die For Schleife alle Schlüssel aus  %{$attr{$d}} - abzüglich derer in @dontSave durchlaufen, wobei diese Restschlüssel alphabetisch sortiert sein sollen mit der Spezialität, dass userattr immer erster Eintrag ist. Auf den zugehörigen Hashwerten zu diesen Schlüsseln, wird ein wenig Escaping-Geknödel betrieben, in einer @ret Liste gespeichert und diese zurückgegeben. Ja?

Falls ja, dann:

sub GetDefAndAttr {
    my $d         = shift;
    my $dumpFUUID = shift;

    my @ret;

    if ($d ne 'global') {
        my $def = $defs{$d}->{DEF} // '';

        $def =~ s/;/;;/g;
        $def =~ s/\n/\\\n/g;
        push @ret, "define $d $defs{$d}->{TYPE} $def";
    }

    push @ret, "setuuid $d $defs{$d}->{FUUID}"
        if ($dumpFUUID && defined($defs{$d}->{FUUID}) && $defs{$d}->{FUUID});


    # first we build a hash to be able to eliminate exclude attributes in O(1) lookup
    my %dontSave = map { $_ => 1 }
                   qw(configdb:rescue configdb:nostate configdb:loadversion
                      global:configfile global:version);

    # then we generate the list of attributes to iterate in a special sorted way
    # (userattr must be first) and filter the %dontsave combinations out
    my @attrkeys = grep { ! exists $dontSave{"$d:$_"} }
                   sort { return -1 if ($a eq 'userattr');
                          return  1 if ($b eq 'userattr');
                          return $a cmp $b;
                        } keys %{$attr{$d}};

    for my $ak (@attrkeys) {
        my $val = $attr{$d}->{$ak};
        $val =~ s/;/;;/g;
        $val =~ s/\n/\\\n/g;
        push @ret,"attr $d $ak $val";
    }

    return @ret;
}


Der Code ist jetzt zwar nicht kürzer, aber die Komplexität der Schleife ist extrem reduziert, eigentlich bis hin zur trivialität.
Der Aufbau von %dontSave ist übersichtlich, die größte Komplexität liegt im Aufbau der @attrkeys Liste. Da kann man vermutlich
noch was mit dem sort-Block machen.

Aber auch diese Lösung ist ein Beispiel einer Reduktion O(n²) -> O(n): Der Schlüssel liegt im konstanten lookup eines Hashes (exists) gegenüber der linearen iteration einer Liste (grep).
Witty House Infrastructure Processor (WHIP) is a modern and
comprehensive full-stack smart home framework for the 21st century.

RichardCZ

#44
Zitat von: RichardCZ am 04 April 2020, 09:16:15
Hey! 781 Tests.  ;)
Gut, die meisten sind noch von Gentoo::Version, aber immerhin und bald schmeisse ich auch einige von Hobo mit rein.

Kann man das sehen?

https://gl.petatech.eu/root/HomeBot/-/jobs/82

Erste CI Pipeline röchelt schon.  :)




edit:

Ich bin ja so stolz auf mich. Eigentlich sollte ich ja sagen "Wir sind ja so stolz auf uns." Pluralis MaJestatis - wissensschon.  ;)

https://gl.petatech.eu/root/HomeBot/-/jobs/89

1355 Tests.

Mal eben Net::SNMP, Net::MQTT, Readonly mitgeliefert und HomeBot -> HoBo (bei den Modulen umbenannt). Letzteres wäre bereits ohne die einfachen "use smoketests" eine Tortur.
Witty House Infrastructure Processor (WHIP) is a modern and
comprehensive full-stack smart home framework for the 21st century.