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

Analyzer to prefer StringAssert.Contains over Assert.Contains #4832

Closed
radmorecameron opened this issue Jan 29, 2025 · 1 comment · Fixed by #4842
Closed

Analyzer to prefer StringAssert.Contains over Assert.Contains #4832

radmorecameron opened this issue Jan 29, 2025 · 1 comment · Fixed by #4842

Comments

@radmorecameron
Copy link

Summary

Assert.Contains can accept a string parameter and might cause confusion if someone were to use that method over StringAssert.Contains

Background and Motivation

Since String inherits IEnumerable<char>, it's possible to use the new Assert.Contains method with a string and char parameter.
Ex:

Assert.Contains("hi", 'h');

When doing an assertion on a string I think it could cause some confusion since the new method also accepts a string parameter

Proposed Feature

It could be good to have an analyzer to prefer StringAssert.Contains over Assert.Contains with a codefix for switching to StringAssert.Contains

Ex:

StringAssert.Contains("hi", "h"); // note: this currently doesn't support char as second parameter

Alternative Designs

Another option could be to deprecate StringAssert.Contains and copy over the string assertion functionality as a new overload for Assert.Contains

@Evangelink
Copy link
Member

I am personnaly not a big fan of StringAssert and CollectionAssert classes as they hurt a lot API discovery and don't bring much value (in my eyes). So I would vote for more assertions in Assert, deprecating StringAssert is "hard" if we don't have a code refactoring to move people over as the value is small and we force them to change (often fine for OSS users but a major problem for entreprise users).

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

Successfully merging a pull request may close this issue.

3 participants