-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Fixed tariff: fix sorting zones with deviating months or days #26413
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey - I've left some high level feedback:
- The test cases use
m=0andd=0when constructingtime.Date, which will panic because Go months are 1–12 and days start at 1; adjust these to valid values (and, if needed, adjust expectations accordingly). - The slice
tc := []struct{...}{...}and the loop variablefor _, tc := range tcshare the same name, which is confusing and shadows the outer variable; rename one of them for clarity.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The test cases use `m=0` and `d=0` when constructing `time.Date`, which will panic because Go months are 1–12 and days start at 1; adjust these to valid values (and, if needed, adjust expectations accordingly).
- The slice `tc := []struct{...}{...}` and the loop variable `for _, tc := range tc` share the same name, which is confusing and shadows the outer variable; rename one of them for clarity.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| clock := clock.NewMock() | ||
| at.(*Fixed).clock = clock | ||
|
|
||
| clock.Set(time.Date(2025, time.Month(tc.m), tc.d, 0, 0, 0, 0, time.UTC)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| clock.Set(time.Date(2025, time.Month(tc.m), tc.d, 0, 0, 0, 0, time.UTC)) | |
| clock.Set(time.Date(2025, time.Month(tc.m), tc.d, 0, 0, 0, 0, time.Local)) |
richtig?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure. Muss einfach die gleiche Zeitzone sein wie die mack clock. Am besten von dort nehmen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
die Raten arbeiten wohl in Time.Local - dann wäre das richtig; bzw. auch die mock clock; soweit ich das finden konnte, wäre UTC hier falsch.
Wenn du also keinen konkreten Grund hast für UTC hier, nehmen wir besser Local.
Die Zeiten würden ansonsten "springen"... - hab einen Ansatz -> "wip"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…oder wir sorgen dafür, dass die Raten in der TZ der clock arbeiten, die dann normalerweise local sein sollte.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wird explizit eine "Reihenfolge" in der Spezifizierung der Tarife erwartet?
Hintergrund: wenn nein, braucht es eine Sortierung, die das berücksichtigt.
erlaubt:
zones:
- hours: 0-5, price: 0.20
- hours: 0-5, months: Oct-Dec, price: 0.10
- hours: 0-5, days: Mon-Fri, price: 0.15
- hours: 0-5, months: Oct-Dec, days: Mon-Fri, price: 0.05
zones:
- hours: 0-5, price: 0.20
- hours: 0-5, months: Oct-Dec, price: 0.22
nicht erlaubt (spezifischere Angabe zuerst):
zones:
- hours: 0-5, months: Oct-Dec, price: 0.22
- hours: 0-5, price: 0.20
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Angabe ist egal. Es sollte intern nach Spezifität sortiert werden.
|
wip in #26425, no write access |
Fix #26164