Roslyn: LanguageVersionFacts adds a TryParse() extension to the String class

[bbzippo]

https://github.com/dotnet/roslyn/blob/944ae114140ba77cbd4be370bf13a7f758f740b3/src/Compilers/CSharp/Portable/LanguageVersion.cs#L240

Please make it a regular static method.

[Neme12]

How is it not a regular static method? Are you aware that you can still use it as one...?
c# LanguageVersionFacts.TryParse("7.1", out var version);

[bbzippo]

Please refer to
https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/extension-methods

Making TryParse an extension of String is misleading and breaks conventions. When you type "myString." and see "TryParse" in the intellisense, that's not very nice.

[Neme12]

It wasn't clear from the original post what your concern is. Please be more descriptive next time.

[jcouv]

Although I agree with the proposal, this API is part of the public surface area, so changing it would be a compatibility break.

[bbzippo]

Sure, it would be a breaking change, so we should not only fix this, but also document it in the release notes.
The risk of breaking somebody's code is negligible: who would use string.TryParse() to parse versions?
The risk of not fixing it may also be small, but it is persistent: developers will always be seeing a misused language feature coming from the language designers themselves.
The effort to fix is very small.
So why "won't fix"?

[jcouv]

I marked as won't fix because we avoid compat breaks unless strong justification. That said, I'll double-check with compat council.

[jcouv]

I had another thought. What if I marked this method as not browsable from the editor (there's an attribute for that). Then it would be available (no break) but it would not pollute the completion list.

[bbzippo]

That's a good idea, however, wouldn't that also hide it from the member list of LanguageVersionFacts?
The non-breaking way to deal with this is to mark this method as deprecated and non-browsable, and add a static (not extension) method with a different name, e.g. TryParseVersion()

[jcouv]

Marking a method a obsolete is a breaking change too... :-(
I think non-browsable should be enough, as the main downside is that one has to type the method name by hand.

[bbzippo]

I agree that would be a reasonable mitigation

[bbzippo]

jcouv, thanks for handling this small issue professionally.

[jcouv]

My pleasure. Thanks for the feedback.

[jcouv]

@bbzippo We ended up making TryParse a non-extension method after all. The compat council accepted the break after all. Thanks