-
Notifications
You must be signed in to change notification settings - Fork 4
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
fix: parse time function for nedlib
#139
Conversation
nedlibreader/time.go
Outdated
return | ||
} | ||
|
||
err = fmt.Errorf("failed to parse string as time.Time: %s: %v", dateString, err) |
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.
err = fmt.Errorf("failed to parse string as time.Time: %s: %v", dateString, err) | |
err = fmt.Errorf("failed to parse string as time.Time: %s: %w", dateString, err) |
Wrap error to be able to use errors.Is
or errors.As
.
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.
This function will return the error message from the latest attempt which is a bit misleading considering we are trying different layouts.
Maybe we should return
time.ParseError{
Message: ": cannot parse as [list of formats?]"
}
considering:
// Error returns the string representation of a ParseError.
func (e *ParseError) Error() string {
if e.Message == "" {
return "parsing time " +
quote(e.Value) + " as " +
quote(e.Layout) + ": cannot parse " +
quote(e.ValueElem) + " as " +
quote(e.LayoutElem)
}
return "parsing time " +
quote(e.Value) + e.Message
}
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.
The error message is misleading, I agree. We should probably improve it, but right now the current implementation of this PR is better than what exists in main
.
I added a fixup to fix the error handling in the invalid time format cases, so now we only check for the type, not the actual error message.
nedlibreader/time_test.go
Outdated
|
||
func TestParseInvalidRFC1123(t *testing.T) { | ||
dateString := "Tue, 05 Apr 2024 15:30:00" | ||
expectedError := "failed to parse string as time.Time: Tue, 05 Apr 2024 15:30:00: parsing time \"Tue, 05 Apr 2024 15:30:00\" as \"Mon Jan _2 15:04:05 MST 2006\": cannot parse \", 05 Apr 2024 15:30:00\" as \" \"" |
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.
Better to compare error against time.ParseError
using errors.As
or errors.Is
.
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.
Fixed in a fixup
f2da2b6
to
c7a428a
Compare
Ready for next round @maeb |
c7a428a
to
3aacc01
Compare
After an IRL review with @maeb, we decided to return an additional argument from |
Ready for next round @maeb |
# Motivation `nedlib` data might contain a lot of different time formats. This commit adds a function to parse the time format used in `nedlib` data. # Future work Add parsing of custom time formats.
Motivation
nedlib
data might contain a lot of differenttime formats. This commit adds a function to parse the time format used in
nedlib
data.Future work
Add parsing of custom time formats.