Skip to content

EqualExportedValues: Support nested structs and time.Time fields#1373

Closed
feidtmb wants to merge 7 commits intostretchr:masterfrom
feidtmb:master
Closed

EqualExportedValues: Support nested structs and time.Time fields#1373
feidtmb wants to merge 7 commits intostretchr:masterfrom
feidtmb:master

Conversation

@feidtmb
Copy link

@feidtmb feidtmb commented Apr 11, 2023 •
edited
Loading

Summary

Updates the new EqualExportedValues function to support comparing nested struct fields and time.Time values. The function explicitly no longer supports recursive data structures (see this since removed test case for example).

Changes

#1309 introduced EqualExportedValues. I was looking to add support for comparing structs containing time.Time fields, and this seemed like an ideal function to adjust for that since:

  1. The explicit comparison of only exported values suggests the caller doesn't want a typical deep equality check, so comparing times using time.Time.Equal is likely more in line with the desired behavior.
  2. EqualExportedValues is not yet in a tagged release, and so this behavior change should not affect many real users.
  3. Past PRs have tried to add time.Time equality support to functions like assert.Equal, but that task was found to be more complicated (for example, see Fix compairsion of struct containing time.Time.  #985). Adding this support to only EqualExportedValues is more easily achieved.

Motivation

There is currently no good method for comparing structs that contain time.Time fields that don't have matching timezones or monotonic clock readings.

Example Usage:

type Q struct {
	TS *time.Time
}
type R struct {
	Nested Q
}
type S struct {
	Nested R
	CreatedAt time.Time
}

t1 := time.Date(2023, time.April, 7, 12, 56, 32, 0, time.UTC)
t2 := time.Date(2023, time.April, 7, 7, 56, 32, 0, time.FixedZone("UTC-5", -5*60*60))

s1 := S{Nested: R{Nested: Q{TS: &t1}}, CreatedAt: t1}
s2 := S{Nested: R{Nested: Q{TS: &t2}}, CreatedAt: t2}

ObjectsExportedFieldsAreEqual(S1, S2) => true

Related issues

Potentially closes #984, assuming we're okay with requiring use of EqualExportedValues to compare structs with time.Time fields and don't expect assert.Equal to handle that.

@feidtmb feidtmb marked this pull request as ready for review April 11, 2023 13:47
@feidtmb
Copy link
Author

feidtmb commented Apr 11, 2023

CC @mchlp since this proposes changes to your new functions from #1309.

@feidtmb
Copy link
Author

feidtmb commented Apr 11, 2023

Of course right after I undraft this I realize recursive structs need to be handled...

Looking into that and will undraft again when ready.

@feidtmb feidtmb marked this pull request as draft April 11, 2023 13:56
@HaraldNordgren
Copy link
Contributor

HaraldNordgren commented Apr 25, 2023 •
edited
Loading

Hi @feidtmb, I was also looking into the issues of the recursive structs (my entry point was the issues with pointers and slices). Here is a fix I proposed, if you want to take a look: #1379

If you think it makes sense, I we could move the time.Time logic in there too another case in the switch logic 🤗

@mchlp
Copy link
Contributor

mchlp commented Apr 26, 2023

I think EqualExportedValues is the right place to put the time comparisons with time.Time.Equal. However, maybe EqualExportedValues would no longer be an accurate name for the function since we're doing more than just comparing the exported values. Maybe EqualPublicInterface?

@feidtmb
Copy link
Author

feidtmb commented Apr 27, 2023

@HaraldNordgren I left a review on your PR. It looks good to me, thanks for tackling the recursive structures problem!

I've removed the expectation for this PR to handle recursive data structures, and will leave it as a draft for now with the assumption that your PR will be merged instead. Once that's done I'll either close this or adjust it to handle only time.Time equality like you mentioned.

@feidtmb
Copy link
Author

feidtmb commented Apr 28, 2023 •
edited
Loading

@HaraldNordgren Looking over your PR again, while I do think your approach is more complete than this one since it handles maps and arrays, I don't think it's handling recursive data structures. (See this test case.)

Just wanted to call that out since I was mistaken in my comment above. (I left a similar comment on your PR.) Still happy to proceed with your solution though.

@HaraldNordgren
Copy link
Contributor

HaraldNordgren commented Apr 29, 2023

Hi @feidtmb!

In a real scenario in don’t see a case where there are objects referencing themselves. I don’t think I have ever seen them in production code. This I why I wouldn't want to add a comment explaining the lack of this. But maybe I’m wrong?

Regarding a possible solution to it: One level of recursion should be simple to solve. But if we have multiple levels of data before the structure refers back to top level, then it becomes difficult to solve, where we have to keep track of all previous levels. Probably not impossible, but much more complex to write.

If we want to do it, maybe all of this is better suited for a follow-up PR. What do you think? 🤗

@HaraldNordgren HaraldNordgren mentioned this pull request Apr 29, 2023
Merged
@feidtmb feidtmb closed this May 19, 2023
@Antonboom Antonboom mentioned this pull request Sep 14, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Struct containing a time.Time can't be compared correctly.

3 participants

@feidtmb @HaraldNordgren @mchlp