fix(octopus): review follow-ups for #4849, #4850 and #4852 - #4873
Open
mgazza wants to merge 3 commits into
Open
Conversation
Review nits: the test docstring skipped from Test 7 to Test 9 despite Test 8 existing, and "block_count'th" read awkwardly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NnzhfR8uDgGUtYLNpDA4ka
Review nit: the new backoff/token-request logs read "Warn: Kraken token ..." while the rest of the component uses "Warn: Kraken: ...". The odd one out breaks grep and log-parsing on the usual prefix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NnzhfR8uDgGUtYLNpDA4ka
Review follow-up (@gcoan). A Weekend Happy Hour cannot be joined through the API - Octopus either allocates one or the user books it on the website - so listing it in available_events only produces join attempts the API rejects (#4593/#4595) and fills the join selector with options that cannot be selected. BottleCapDave's integration stopped passing these through in v19.0.1 for the same reason; match that now that eventType makes them identifiable. Ordering matters and is easy to get wrong: the reward/code/type maps are built from this same events list, and a joined Happy Hour looks its event type up there. Skipping before those maps are filled leaves the joined event with no type and the injected default reward, so the planner prices a free hour as an 80p/kWh saving session. The skip therefore sits after the maps are populated, and the test fails if it is moved above them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NnzhfR8uDgGUtYLNpDA4ka
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-ups to #4849, #4850 and #4852. All three merged quickly — thank you — but these landed on the branches just after the merges, so they need a fresh PR.
1.
fix(octopus): stop offering Weekend Happy Hours as available to join — @gcoan's request on #4851:Now that
eventTypeis being requested (#4852) these are identifiable precisely, so Octopus Direct can match BCD. Listing them only produces the join attempts the API rejects (#4593/#4595) and fills the join selector with options that cannot be selected.Ordering trap worth a reviewer's eye, because it is invisible in the diff. The reward/code/type lookup maps are built from the same
eventslist, and a joined Happy Hour reads its type out of those maps. Skipping before they are populated leaves the joined event withevent_type: Noneand the injected default reward — so a free hour is priced as an 80p/kWh saving session, which is worse than the bug #4852 fixed. The skip therefore sits after the maps are filled, andtest_octopus_saving_event_typefails if it is moved above them.2.
style(kraken): use theKraken:log prefix — Copilot on #4850. The new backoff and token-request warnings readWarn: Kraken token …while the rest of the component usesWarn: Kraken: …; the odd one out breaks grep and log parsing on the usual prefix.3.
docs(octopus): test list numbering and docstring phrasing — Copilot on #4849. The test docstring skipped Test 7 → Test 9 despite Test 8 existing, and "block_count'th" read awkwardly. Note the docstring moved toutils.pyin #4850, so the fix is applied there.Copilot's third point on #4849 was that the PR description undercounted the tests — that was a description issue on a now-merged PR, so there is nothing to change in the code.
Tests
New
test_octopus_saving_event_type, registered inunit_test.py:available_eventsbut still reachesjoined_eventscarryingevent_type: WEEKEND_HAPPY_HOURTURN_DOWNalongside it is still offered, with its typeeventTypeis unaffectedunit_test.py -k octopus(72),-k savingand-k krakenall pass.