Skip to content
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

Don't delete a time field if we can't parse it as a time. #263

Merged
merged 4 commits into from
Aug 26, 2022
Merged

Conversation

kentquirk
Copy link
Contributor

@kentquirk kentquirk commented Aug 25, 2022

Which problem is this PR solving?

Short description of the changes

  • GetTimestamp violates its documented behavior (fails to delete the timestamp field) if a timestamp matches a certain format. This causes it to do that.
  • GetTimestamp looks at a bunch of fields and tries to parse them as a timestamp. If it succeeds, it deletes the field. But there was a bug (see the referenced bug report). This fixes that bug.

@kentquirk kentquirk requested review from a team and pkanal August 25, 2022 16:50
ts = tryTimeFormats(timeStr, timeFieldFormat)
if !ts.IsZero() {
// Only set foundFieldName if we were able to parse it as a time
foundFieldName = timeField
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we add a test for this if possible?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call. Added.

Copy link
Contributor

@MikeGoldsmith MikeGoldsmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seal-of-approvement

@kentquirk kentquirk merged commit 5d8dfeb into main Aug 26, 2022
@kentquirk kentquirk deleted the kent.239 branch August 26, 2022 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants