nunnatsa / ginkgolinter

golang linter for ginkgo and gomega
MIT License
24 stars 6 forks source link

[Enhancement] Should `Expect` be matched with `To`? #112

Closed rmohr closed 4 months ago

rmohr commented 9 months ago

Is your feature request related to a problem? Please describe.

I wonder if something like

Expect(myPath).ShouldNot(BeEmpty())

should be automatically improved.

Describe the solution you'd like

It feels like Expect should always be matched with a To*.

For example:

Expect(myPath).ToNot(BeEmpty())

And Eventually and Consistently should be matched with Should*.

For example:

Eventually(myPath).ShouldNot(BeEmpty())

Describe alternatives you've considered

Additional context

Another example:

Expect(err).To(BeNil()) instad of Expect(err).Should(BeNil())

Maybe worth an optional suggestion for auto fixing?

nunnatsa commented 8 months ago

I'm not sure about this, but don't have strong feelings about it. My main concern is for existing code. This change will force many projects to fix 1000s lines, and honestly, it does not look for me too important to cause this mess.

@onsi - is there a reason for having these multiple variants? Do you think user should prefer one over another?

onsi commented 8 months ago

The reasons are not very good and if I were to start over I would probably be more disciplined.

Gomega has two mechanisms for dialects for assertions: Ω(foo).Should(Bar()) and Expect(foo).To(Bar()) - but bother Ω and Expect return the same object which has both the Should and To methods.

At the end of the day they're all fairly clear - but I can understand that the inconsistency (and poor english) of different variants might drive some people crazy. Still, making this a default linter behavior will push 1000s of lines of code into an error state which may not be what you want. Making it off by default but configurable for users who want it might be a good compromise.

Someday Gomega 2 might be a reality where this gets cleaned up. But there are no plans for that yet.

rmohr commented 5 months ago

Making it off by default but configurable for users who want it might be a good compromise.

Yeah that sounds good. Just an easy way to clean it up with the linters fix option is mostly what I am looking for.

mouhsen-ibrahim commented 5 months ago

Hi, I would also like to have this feature and be configurable, I'm already refining my code base to use To with Expect and Should with Eventually. So adding this feature will really help me to finish this refactoring and have a clear standard followed in the whole code base.

rmohr commented 4 months ago

Thanks @nunnatsa this is awesome. Should we do the same for Eventually/Consistently with Should?

nunnatsa commented 4 months ago

Should we do the same for Eventually/Consistently with Should?

We don't need to, because these functions don't support To/ToNot/NotTo anyway.

rmohr commented 4 months ago

Oh. Nice! Thanks again for addressing!