Remove `StringUtil.Compare()` and update references in the tests

[stevenaw]

The StringUtil.Compare() function is public but lives in the "Internal" namespace. It isn't actually used in any constraints, but there are three references in the tests still to it. A comment on the class indicates that

Provides methods to support legacy string comparison methods.

I'd like to propose we begin the process of removing this. I think we can convert the test usage already but I'd like to suggest we mark the method itself as obsolete rather than outright removal even though it's in the internal namespace to maximize others' chance to move away from it.

@nunit/framework-team I think this could be a good first issue if we can agree on if we should obsolete or remove outright

[OsirisTerje]

Supports just removing it.

[manfred-brands]

I'm also in favour of removing it. It is just not worth it to doing it in phases. Technically it is a breaking change, but practically nobody should have used it.

[stevenaw]

Ok that works for me too.

Interestingly, I looked through the history and these made their way into the tests as a result of being used in a bug repro. Which means at least one person at one time has relied on these. I can not find it documented in our docs however, so I agree the number impacted will be extremely small.

[sergioylee]

Hey, I'd love to take a shot on this one 😊

[OsirisTerje]

@mr-sergito Feel free :-)

[sergioylee]

Been reading through the contributing guidelines.

If I'm understanding things correctly, removing method StringUtil.Compare() involves removing its class StringUtil right?

Regarding the tests that reference both methods in the class, should I just replace them with System.StringComparison?

[stevenaw]

@mr-sergito That's right! Part of removing this would involve updating the callsites in the tests to use that built-in methods + enum.
I'd recommend trying to find the affected tests, make sure you can run them and they pass, then make the change to update just that Compare method call 🙂

Removing the entire StringUtil class itself could then be a good next step if the team agrees. On my end, I think it makes sense that if we are ok with deleting one reference then we are ok with deleting the one method then we are ok with deleting the other too. From what I can recall the remaining Equals() method may be called from 2 or 3 places in the framework itself in some path-related constraints.

@nunit/framework-team objections or agreement to remove the whole class?

[OsirisTerje]

objections or agreement to remove the whole class?

We're assuming then that no one outside is actually using that class/method. I believe it is very unlikely someone do, but if that assumption is wrong..... Also, using NUnit to get hold of some string utility method seems very odd. So, yes, all in all, let us just get rid of it.

UPDATE:

I searched github for language:csharp "StringUtil.Compare(" and found 83 matches, and 78 of these where from forks of the framework, the remaining 5 is either old or have their own definition.

So wrt to open sourced code, we're good. And I think that is good enough.

[stevenaw]

As an aside, as an "internal" method if concerns were present, I'd suggest that marking something with the obsolete attribute could then be candidate for removable in a future minor version release rather than waiting for the next major release. Depending on usages or visibility, of course.

EDIT: Though it sounds like immediate removal may be tolerable here