Antonboom / testifylint

The Golang linter that checks usage of github.com/stretchr/testify.
https://github.com/stretchr/testify
MIT License
97 stars 9 forks source link

regexp: simplify `assert.Regexp(t, regexp.MustCompile)` #82

Closed ccoVeille closed 2 months ago

ccoVeille commented 5 months ago

@dolmen post helped me to spot a new checker that could be added

https://github.com/Antonboom/testifylint/issues/81#issuecomment-2060618745

assert.Regexp(t, regex.MustCompile("whatever")… could be simplified into assert.Regexp(t, "whatever"…

I spotted it here https://github.com/SCP002/m3u_merge_astra/blob/9ae08f40f415292bf646a2dd7811f4d5d799dea6/src/util/logger/logger_test.go#L23

ccoVeille commented 5 months ago

Also cc @kevinburkesegment who somehow initiated my discovery

Related: stretchr/testify#1585

mmorel-35 commented 2 months ago

Grafana has a fork from regexp, see https://github.com/grafana/regexp

It might be intrested to suggest it for the standard library and grafana’s version as well ?

ccoVeille commented 2 months ago

Grafana has a fork from regexp, see https://github.com/grafana/regexp

It might be intrested to suggest it for the standard library and grafana’s version as well ?

I'm unsure to get your point.

There are some interesting lib for regexp.

But, here we are in a linter specialized in: tests (so perf is not the priority) and testify.

Testify has an asserter for Regexp. This issue is about converting

assert.Regexp(t, regex.MustCompile("whatever"))

// simplified
assert.Regexp(t, "whatever")
kevinburkesegment commented 2 months ago

I do not think that adding a new dependency would simplify things

mmorel-35 commented 2 months ago

grafana's regexp is implementing the regexp.MustCompile(...) see https://pkg.go.dev/github.com/grafana/regexp#MustCompile

I was not talking about adding a dependency to testifylint. As testifylint is able to make recommandation for errors or github.com/pkg/errors without needing to depend on github.com/pkg/errors (see https://github.com/Antonboom/testifylint/blob/858de4660f5794af36f317dbc2362b1f5c411d2b/internal/analysisutil/object_test.go#L120). It shall be possible for testifylint to recommend the adapted usage if regexp.MustCompile() is coming from regexp or from github.com/grafana/regexp

ccoVeille commented 2 months ago

Now, I get it.

I didn't check the implementation, but I'm doubtful assert.Regexp works with something else than std lib.

Edited: checked

https://github.com/stretchr/testify/blob/v1.9.0/assert%2Fassertions.go#L1613-L1623

Did you find examples of such usage?

Or this lib is used with a replace hack in go.mod?

mmorel-35 commented 2 months ago

I know that Prometheus is using grafana's lib, see https://github.com/search?q=repo%3Aprometheus%2Fprometheus%20github.com%2Fgrafana%2Fregexp&type=code

I'm not sure they have a test with it but in any case it could apply with the same recommandation . The difference is in the performance so in case if a test in doesn't matter

ccoVeille commented 2 months ago

I think I understand what you meant. But, unfortunately when I consider it, I don't understand what you expect, because for me, it's impossible to use graphana/regexp.MustCompile with testify.

My point is that graphana/regexp.MustCompilereturns a graphana/regexp.Regexp, so not a regexp.Regexp that is only type assert.Regexp is able to use.

Yes, there is something to handle pkg/errors and errors but these package were equivalent. Your suggestion will be OK if and only if testify was able to support graphana.Regexp. Something I believe to be impossible now I checked the testify code. I see nothing about an interface. So current code about people using assert.Regexp with graphana.Regexp will have invalid code, I mean code that doesn't work as expected. I might be wrong, or I have misunderstood something.

So, I will step out and let other reply. Because, I cannot help

Antonboom commented 2 months ago

Please, stop this discussion. testifylint should work with std lib only or libs that inherited from testify's assertions signatures.

mmorel-35 commented 2 months ago

Thanks to @ccoVeille , I understand that my suggestion just doesn’t apply.