fix: time.Time rendering in the diff returned by error message#1829
Open
ccoVeille wants to merge 1 commit intostretchr:masterfrom
Open
fix: time.Time rendering in the diff returned by error message#1829ccoVeille wants to merge 1 commit intostretchr:masterfrom
ccoVeille wants to merge 1 commit intostretchr:masterfrom
Conversation
Now we vendored go-spew we can modify how time.Time values are handled. We can remove the imperfect fix we had in place before.
3f71fbe to
7b28236
Compare
Collaborator
Author
|
@brackendawson please review, it's ready. I'm facing this issue with testify for ages, I cannot wait to see it fixed 😬 |
fredbi
added a commit
to fredbi/testify
that referenced
this pull request
Jan 4, 2026
This is inspired by stretchr#1829, but we proceed differently, not checking for a string type but for type convertibility to a time instead. Added more tests to check how embedded types, pointers etc actually render and don't cause panic. From the original issues: * fixes stretchr#1078 * fixes stretchr#1079 Signed-off-by: Frederic BIDON <fredbi@yahoo.com>
|
@ccoVeille I found your PR definitely interesting but perhaps lacking a few things.
Could be interesting to exchange our views |
Collaborator
Author
|
Thanks @fredbi. I will take a look. I saw the PRs you made in the last days. I agree with you about the string sort that can lead to problem when comparing for example 999-12-31 and 1000-01-01 I'll review the PR in your fork also |
fredbi
added a commit
to go-openapi/testify
that referenced
this pull request
Jan 4, 2026
* fix: rendering time values * added sortability for time.Time This is inspired by stretchr#1829, but we proceed differently, not checking for a string type but for type convertibility to a time instead. Added more tests to check how embedded types, pointers etc actually render and don't cause panic. From the original issues: * fixes stretchr#1078 * fixes stretchr#1079 * doc: fixed a few hiccups in README Signed-off-by: Frederic BIDON <fredbi@yahoo.com> --------- Signed-off-by: Frederic BIDON <fredbi@yahoo.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Improve formatting when dealing with time.Time now we vendored gospew
Changes
Now, go-spew is vendored, we can fix old issues we had.
We can fix the representation of time.Time in gospew
And remove the imperfect fix we had in place.
Motivation
#895 was created by @luan to solve some legitimate issues. It was released with v1.6.0 on May 29, 2020
But unfortunately, the changes lead to an issue with the way time.Time are rendered. So #989 was opened on Aug 1, 2020 by @AlekSi
The issue was somehow fixed with #1072 by @HaraldNordgren
#1072 was merged in Apr 27, 2021 (but only released in Mar 1, 2024 with v1.7.1 )
Unfortunately, the fix was incomplete, as it was working only for first level, so #1078 was opened on May 22, 2021 by @shawc71
#1079 was opened by @shawc71 on May 22, 2021 to try to fix the issue
But it was never merged, as the solution was also imperfect, and because they were no solution inside testify to fix this
So #1078 was left open until now.
Related issues