Planner: simple planning on incomplete rates

[iseeberg79]

follow-up on discusson #24423

[iseeberg79]

test fails, but not related but interessting...

[sourcery-ai]

Hey - I've left some high level feedback:

• Consider renaming TestContinuous_StartBeforeRatesInsufficientTime (and possibly its description) to reflect that the behavior is now to start at the latest possible time before the rates window rather than immediately, so the test name matches the updated semantics.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- Consider renaming `TestContinuous_StartBeforeRatesInsufficientTime` (and possibly its description) to reflect that the behavior is now to start at the latest possible time before the rates window rather than immediately, so the test name matches the updated semantics.

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[andig]

Wenn der Fall eintreten kann dann aber analog auch für "nicht continuous". Wieso behandeln wir den dann nicht vorher oben?

[iseeberg79]

ja, du hast recht! das erspart hier die Prüfung und sorgt für konsistentes Verhalten für beide Strategien.
Wenn nicht genug Raten verfügbar sind, wird aktuell für non-continuous nicht geprüft, ich dachte - da greift simplePlan bereits.

Es sollte bei nicht ausreichend verfügbaren Tarifdaten und ungenügender Zeit einen simplePlan als Antwort geben?

Es gibt einen Testfall, der das "falsche" Verhalten erwartet:
require.NotEmpty(t, plan, "plan should not be empty - starts when rates become available")

Das würde dann aber tatsächlich dazu führen, das der non-continous eine Stunde zu spät mit der Ladung beginnt?

Verhalten und Test anpassen, ja?

[iseeberg79]

Test war vor dem continuous Fall offen, ich hab das aktuelle Verhalten ausgedrückt.
Lass uns das anpassen und für beide Fälle bei nicht ausreichend verfügbaren Tarifdaten den simplePlan nutzen

[sourcery-ai]

Hey - I've left some high level feedback:

• The new rate coverage check uses rates[len(rates)-1].End.Sub(rates[0].Start) which ignores both now/targetTime clamping and any gaps inside the window; if the intention is to check the actual usable coverage, consider reusing the clamped start/end logic or explicitly accounting for gaps rather than just span.
• Previously, insufficient coverage in continuous mode fell back to continuousPlan, but now the early check returns simplePlan for both continuous and dispersed; if this behavioral change is intentional it might be worth making that explicit in code/comments, otherwise consider preserving the earlier continuous-specific fallback.
• The comment // check if rate coverage is sufficient for planning is quite generic; consider tightening it to describe the exact condition (e.g., that the total span of rate data must cover at least requiredDuration or the planner ignores rates) to avoid misinterpretation during future refactors.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The new rate coverage check uses `rates[len(rates)-1].End.Sub(rates[0].Start)` which ignores both `now`/`targetTime` clamping and any gaps inside the window; if the intention is to check the actual usable coverage, consider reusing the clamped start/end logic or explicitly accounting for gaps rather than just span.
- Previously, insufficient coverage in continuous mode fell back to `continuousPlan`, but now the early check returns `simplePlan` for both continuous and dispersed; if this behavioral change is intentional it might be worth making that explicit in code/comments, otherwise consider preserving the earlier continuous-specific fallback.
- The comment `// check if rate coverage is sufficient for planning` is quite generic; consider tightening it to describe the exact condition (e.g., that the total span of rate data must cover at least `requiredDuration` or the planner ignores rates) to avoid misinterpretation during future refactors.

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[andig]

Len(rates) muss hier immer größer 0 sein, oder?

[andig]

Dennoch: len>0 ist hier schon sicher gestellt: https://github.com/evcc-io/evcc/pull/26324/changes#diff-aaad52d4c2a2116a876ad00b56b5959c94db6cbed386b2942f956d044b0ff7c2R125. Lass uns nichts doppeln, das verdeckt nur die echten Probleme.

[iseeberg79]

ja, siehe oben; aber ich glaube die Prüfung ist notwendig, falls clampRates doch die Rates leert. Es würde was merkwürdiges in der Prüfung beim Array-Zugriff passieren..., lass uns das zur Absicherung der Bedingung drin lassen

[andig]

Ungern. Wenns die braucht hätte ich gerne einen Test dafür. Ich sehe aber nicht, wie man in den Fall kommen kann.

[iseeberg79]

Der Fall wären wohl verfügbare Raten, vor dem aktuellen Zeitpunkt. Das wird anders nicht abgeprüft. Die Anpassung sollte im Verhalten konsistent und sicher sein und einen panic verhindern:

Suggested change

	if len(rates) > 0 && rates[len(rates)-1].End.Sub(rates[0].Start) < requiredDuration {
	if len(rates) == 0 || rates[len(rates)-1].End.Sub(rates[0].Start) < requiredDuration {

Einen Test würde ich ergänzen?

[andig]

ok- aber das sollten wir dann ganz oben abfangen, nicht hier unten wieder neu mit dem Thema anfangen:

	// treat like normal target charging if we don't have rates
	if len(rates) == 0 || err != nil {
		return simplePlan
	}

[andig]

diese verteilte Logik ist die Hölle. Einmal konsistente Bedingungen herstellen, dann drauf verlassen.