new checker contains
#152
Merged
new checker contains
#152
Conversation
This file contains 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
Pull Request Test Coverage Report for Build 9632081552Details
💛 - Coveralls |
|
I believe #47 could be a subcase of contains but I'm not so confident with providing it in a error-compare rule |
Because of the strange use cases I reported here, I'm unsure. But they are definitely close yes |
ccoVeille
reviewed
Jun 23, 2024
Pull Request Test Coverage Report for Build 9635529714Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9635616469Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9635658244Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9635733489Details
💛 - Coveralls |
ccoVeille
approved these changes
Jun 23, 2024
analyzer/testdata/src/checkers-default/contains/contains_test.go
Outdated
Show resolved
Hide resolved
Pull Request Test Coverage Report for Build 9636072731Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9636090503Details
💛 - Coveralls |
ccoVeille
reviewed
Jun 23, 2024
mmorel-35
force-pushed
the
contains
branch
2 times, most recently
from
ae198ed
to
e9699f2
Compare
Pull Request Test Coverage Report for Build 9639678060Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9639712727Details
💛 - Coveralls |
ccoVeille
reviewed
Jun 24, 2024
|
LGTM 👍, a few stylistic remarks |
Pull Request Test Coverage Report for Build 9639996723Details
💛 - Coveralls |
|
You did the changes but didn't regenerate and commit the test and golden files |
|
I did from my phone, so I have no access to a terminal. The CI shall be doing the warning |
yes, I agree it should. Something to be added, because it's a common mistake, I did it too. I don't see something that could catch it right now what are your thoughts @Antonboom ? |
ccoVeille
suggested changes
Jun 24, 2024
mmorel-35
force-pushed
the
contains
branch
3 times, most recently
from
0ccbcb1
to
30d2666
Compare
Pull Request Test Coverage Report for Build 9656918190Details
💛 - Coveralls |
Co-authored-by: ccoVeille <3875889+ccoVeille@users.noreply.github.com>
Pull Request Test Coverage Report for Build 9656928741Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9656934424Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9656951923Details
💛 - Coveralls |
ccoVeille
reviewed
Jun 25, 2024
Pull Request Test Coverage Report for Build 9666920543Details
💛 - Coveralls |
Antonboom
approved these changes
Jun 25, 2024
| } | ||
| } | ||
|
|
||
| func TestContainsNotContainsReplacements_Bytes(t *testing.T) { |
There was a problem hiding this comment.
added debug tests to be sure that this is not supported
There was a problem hiding this comment.
How do you prevent these failing tests to fail the go test ./...
I don't get where is the magic?
|
Cannot run on my codebase for the moment, but LGTM 👍 |
ccoVeille
reviewed
Jun 25, 2024
| "github.com/stretchr/testify/assert" | ||
| ) | ||
|
|
||
| func TestContainsNotContainsReplacements_String(t *testing.T) { |
There was a problem hiding this comment.
I might have missed something but I would expected this here
Suggested change
| func TestContainsNotContainsReplacements_String(t *testing.T) { | |
| func TestContainsContainsReplacements_String(t *testing.T) { |
There was a problem hiding this comment.
No, Contains + NotContains
consistent with TestSameNotSameReplacements
There was a problem hiding this comment.
When you know it, you know it 😂🤣
It's obvious now.
Thanks
| } | ||
| } | ||
|
|
||
| func TestContainsNotContainsReplacements_Bytes(t *testing.T) { |
There was a problem hiding this comment.
How do you prevent these failing tests to fail the go test ./...
I don't get where is the magic?
because of |
|
Thanks I learned something today. I had no idea the testdata folder I had on my codebase was not a convention in our codebase but something idiomatic |
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.
Closes #54