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

fix: negative positive with type conversion #212

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ccoVeille
Copy link
Contributor

The code was too naive, and was stripping any code with only one arg

Fixes #210

@coveralls
Copy link

coveralls commented Jan 4, 2025

Pull Request Test Coverage Report for Build 12617782863

Details

  • 26 of 27 (96.3%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.01%) to 93.838%

Changes Missing Coverage Covered Lines Changed/Added Lines %
internal/checkers/helpers_basic_type.go 10 11 90.91%
Totals Coverage Status
Change from base Build 12184548841: 0.01%
Covered Lines: 2467
Relevant Lines: 2629

💛 - Coveralls

@Antonboom
Copy link
Owner

Hi, @ccoVeille

Len call is not type conversion and we have separate helper for it.

I expected:

  • new type conversion test cases (with untyping support and proofs in debug package), if we have not yet it
  • just len-case support

@Antonboom Antonboom added enhancement New feature or request and removed enhancement New feature or request labels Jan 5, 2025
@ccoVeille
Copy link
Contributor Author

ccoVeille commented Jan 5, 2025

Len call is not type conversion and we have separate helper for it.

I totally agree with you. What I don't get is that comment is irrelevant with the fix I'm doing here

I used Len in the test because len was reported by the person who reported the bug.

The problem is about avoiding suggesting to replace assert.True(t, whatever(s) > 0) to assert.Positive(t, s).

The replacement is OK when it's a numerical conversion. But when whatever is len, func (int) int ..., anything with one argument returning an integer

I hope it's clearer.

I agree with the need of adding test to helpers, but I found none for other helpers, assuming you were not looking for it. I decided to do not fight this dragon. Feel free to add them.

The code was too naive, and was stripping any code with only one arg
@ccoVeille
Copy link
Contributor Author

ccoVeille commented Jan 7, 2025

I'm just noticing that the rules simplification by removing the unnecessary type conversion should/could also be applied to other asserter, such as Zero, Empty, Positive, Negative, Len (maybe with the limitations we already talked about)

assert.Positive(t, int(42))
assert.Zero(t, int(42))
assert.Negative(t, uint(42))
...

Here is a real life example
https://github.com/ARM-software/golang-utils/blob/923b6f8bd8635f72b6045c8bc079d28eaedb7723/utils/retry/retry_test.go#L82

Also the type conversion suggestion is only applied when using the -fix, so maybe it could be part of a dedicated linter.

Another way to consider this PR is to move out the type conversion from this PR and move it to another rule that could

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bad suggested fix for negative-positive
3 participants