Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Go plugin #6672

Merged
merged 6 commits into from Apr 8, 2023
Merged

Go plugin #6672

merged 6 commits into from Apr 8, 2023

Conversation

tisoft
Copy link
Contributor

@tisoft tisoft commented Mar 6, 2023

Implementierung eines Go Plugins, so nah wie möglich am Javascript Plugin.

Der 1. commit ist die eigentliche Implementierung. Im Prinzip alles vom javascript Plugin kopiert. Hauptunterschied: im Javascript wird valgenutzt um einen Parameterwert zu lesen, im Go Plugin wird der Name des Parameters als Variablenname genutzt.

Der 2. commit enthält eine demo.yaml, in der das javascript durch go ersetzt wurde. Sollte sich hoffentlich genauso verhalten, wie vorher. Das könnte man dort vermutlich auch alles noch schöner machen, ich habe versucht es möglichst nah am javascript original zu lassen.

(Basiert auf dem Prototypen, der in #5730 enthalten war. An dem Transformation Kram arbeite ich auch noch, dauert aber noch ein bisschen 😄)

cmd/demo.yaml Outdated Show resolved Hide resolved
@andig andig added the enhancement New feature or request label Mar 9, 2023
provider/go.go Outdated Show resolved Hide resolved
provider/go.go Outdated Show resolved Hide resolved
provider/go.go Outdated Show resolved Hide resolved
provider/go.go Outdated Show resolved Hide resolved
provider/go.go Outdated Show resolved Hide resolved
cmd/demo.yaml Outdated
console.log(param+":", val);
console.log("state:", JSON.stringify(state));

type State struct {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hier könnte man noch aus State als Typ verzichten und die variable mit state := struct direkt deklarieren?

cmd/demo.yaml Outdated Show resolved Hide resolved
cmd/demo.yaml Outdated
if (state.Batterypower < 0) { state.BatterySoc++; } else {state.BatterySoc--}
if (state.BatterySoc < 10) {state.BatterySoc = 90}
if (state.BatterySoc > 90) {state.BatterySoc = 10}
state.BatterySoc;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Funktioniert das wirklich? Muss da nicht ein return rein? Generell- warum funktionieren die Getter ohne Return?

cmd/demo.yaml Outdated Show resolved Hide resolved
cmd/demo.yaml Outdated Show resolved Hide resolved
@andig
Copy link
Member

andig commented Mar 18, 2023

Sehr cooler PR! Bzgl. demo.yaml: ich bin noch unsicher, warum das mit den Gettern überhaupt funktioniert. Ansonsten: sollen wir die evtl. so lassen wie sie ist? Gegen JS spricht ja auch erstmal nichts?

@tisoft
Copy link
Contributor Author

tisoft commented Mar 18, 2023

Die demo.yaml würde ich auch als JS lassen. Mit irgendwas musste ich ja testen. Eigentlich wären ein paar unit tests gut, aber ich kenne mich nicht wirklich mit go aus, um zu wissen wie man die vernünftig schreibt. Muss ich mir mal ansehen.

Zu den fehlenden returns: Scheint ohne zu gehen, wenn ich die yaegi documentation richtig verstehe, gibt der das letzte einfach so zurück, aber auch da wären ein paar testcases sicher nicht schlecht, um zu sehen, was man da genau machen kann.

Die Kommentare zur go syntax in der demo.yaml sehe ich mir die Tage noch an.

@tisoft
Copy link
Contributor Author

tisoft commented Apr 6, 2023

@andig Habe den PR rebased und noch einige der Kommentare behoben.

Was fehlt sonst noch damit das gemerged werden kann?

@andig
Copy link
Member

andig commented Apr 6, 2023

Wenns mach meinen kleinen Vereinfachungen noch passt fehlt egtl. nur noch das Zurückdrehen der demo.yaml?

@tisoft
Copy link
Contributor Author

tisoft commented Apr 6, 2023

Habe es mit deinen Änderungen nochmal bei mir mit der demo laufen lassen und das so soweit gut aus. habe die demo.yaml jetzt wieder rausgenommen und nochmal rebased und gepusht. Ich denke das könnte dann jetzt rein.

@andig andig merged commit c14c873 into evcc-io:master Apr 8, 2023
5 checks passed
@andig
Copy link
Member

andig commented Apr 8, 2023

Klasse PR- vielen Dank!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request infrastructure Basic functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants