Skip to content

assert: add NotZeroThenSetZero and NotNilThenSetNil - #1440

Closed
HaraldNordgren wants to merge 2 commits into
stretchr:masterfrom
HaraldNordgren:ThenSetNil
Closed

HaraldNordgren wants to merge 2 commits into
stretchr:masterfrom
HaraldNordgren:ThenSetNil

Conversation

@HaraldNordgren

@HaraldNordgren HaraldNordgren commented Jul 30, 2023 •

Copy link
Copy Markdown
Contributor

I have used similar versions in my own code for many years. It helps to do assertions of API responses, like this:

clientResponse := someApiRequest(request)

assert.NotZeroThenSetZero(t, &clientResponse.CreateTime)
assert.NotZeroThenSetZero(t, &clientResponse.UpdateTime)

assert.Equal(t, expected, clientResponse)

Could resolve the issue brought up in #1432.

@dolmen

dolmen commented Jul 30, 2023 •

Copy link
Copy Markdown
Collaborator
  1. I don't understand the use case. If this API usage is not obvious, then its place is probably not the core testify.
  2. Implantation requires generics. This is a blocking for now as we still aim to support Go versions below 1.18.

@dolmen dolmen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I do not plan to merge this.

But anyway I choose to give you feedback that might be valuable for another contribution

Also you didn't run "go generate ./.. ".

strategy:
matrix:
go_version:
- "1.17"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Don't change this. This is out of scope.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I changed the minimum version to be >=1.18 in discussion with Boyan Soubachov in this PR: #1379 (comment). Maybe that was a bad change?

Comment thread assert/assertions.go Outdated
Comment thread assert/assertions.go Outdated
@dolmen dolmen added enhancement pkg-assert Change related to package testify/assert generics labels Jul 30, 2023
@dolmen dolmen changed the title Create functions 'NotZeroThenSetZero' and 'NotNilThenSetNil' assert: add NotZeroThenSetZero and NotNilThenSetNil Jul 30, 2023
@HaraldNordgren

HaraldNordgren commented Jul 30, 2023 •

Copy link
Copy Markdown
Contributor Author

I'm sorry you feel you don't want to merge this.

Also you didn't run "go generate ./.. ".

Yes, I noticed that the cannot run now. Seems to be because of generics as well. Should I close the PR?

@dolmen

dolmen commented Jul 31, 2023

Copy link
Copy Markdown
Collaborator

From your description of the use case:

clientResponse := someApiRequest(request)

assert.NotZeroThenSetZero(t, &clientResponse.CreateTime)
assert.NotZeroThenSetZero(t, &clientResponse.UpdateTime)

assert.Equal(t, expected, clientResponse)

This can be rewritten as:

clientResponse := someApiRequest(request)

assert.NotZero(t, clientResponse.CreateTime)
assert.NotZero(t, clientResponse.UpdateTime)
var zeroTime time.Time
clientResponse.CreateTime = zeroTime
clientResponse.UpdateTime = zeroTime

assert.Equal(t, expected, clientResponse)

And I think that is would be more clear that the proposed API.

I think that if you want than feature it is best to copy your implementation in the test package where you use it. That way, the source is immediately available to the reader who may not be familiar with the full testify API. We don't need more niche features.

@dolmen dolmen closed this Jul 31, 2023
@dolmen

dolmen commented Jul 31, 2023

Copy link
Copy Markdown
Collaborator

About the code generation not working with generics code, this is a problem that we must fix before introducing any generics-based API.

@HaraldNordgren
HaraldNordgren deleted the ThenSetNil branch July 31, 2023 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement generics pkg-assert Change related to package testify/assert

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants