resources/resource: Fix GroupByParamDate with raw TOML dates #11706
Conversation
jmooring
force-pushed
the
fix/group-by-param-date-11563
branch
from
e07eca7 to
9412904
Compare
| @@ -55,6 +58,11 @@ func getParam(r Resource, key string, stringToLower bool) any { | |||
| return cast.ToFloat64(v) | |||
| case time.Time: | |||
| return val | |||
| case toml.LocalDate, toml.LocalDateTime: | |||
| if vt, ok := hreflect.AsTime(reflect.ValueOf(val), time.UTC); ok { | |||
There was a problem hiding this comment.
Using reflect here isn't needed. You can just do val.AsTime(time.UTC).
There was a problem hiding this comment.
I couldn't quite figure out how to do what you describe, but hopefully using htime.ToTimeInDefaultLocationE(val, time.UTC) is sufficient.
jmooring
force-pushed
the
fix/group-by-param-date-11563
branch
from
9412904 to
442acb0
Compare
| @@ -55,6 +57,12 @@ func getParam(r Resource, key string, stringToLower bool) any { | |||
| return cast.ToFloat64(v) | |||
| case time.Time: | |||
| return val | |||
| case toml.LocalDate, toml.LocalDateTime: | |||
| val, err := htime.ToTimeInDefaultLocationE(val, time.UTC) | |||
There was a problem hiding this comment.
This is nit picking, but but when you have a known type, converting it to another type (e.g. interface{} or reflect.Value) just so it can be converted back to the type you started with. I don't think it allocates any extra memory in this particular case, but I had to navigate into the ToTime... func to see what it does. I suggest you just call val.AsTime directly.
There was a problem hiding this comment.
Sorry about the iterations here. This was the self-inflicted wound:
case toml.LocalDate, toml.LocalDateTime:
https://go.dev/ref/spec#Type_switches
In clauses with a case listing exactly one type, the variable has that type; otherwise, the variable has the type of the expression in the TypeSwitchGuard.
Which meant I had type any instead of the toml.Whatever type, which meant AsTime() was not an option. Split the case; all is good.
jmooring
force-pushed
the
fix/group-by-param-date-11563
branch
from
442acb0 to
7fe5100
Compare
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Closes #11563