We expect any TryParse to prevent a crash under the application, since .NET uses this standard for all other functions, like int.TryParse or Guid.TryParse. To our surprise, Identity.TryParse does not and uses a try / catch clause, which defeats the purpose of TryParse:
Could Optimizely's team look at that please?
I'm not sure what you meant but Guid.TryParse for example has the internal try/catch. The premise of TryPrase is to not throw exception (instead just return false), and I think Identity.TryParse does just that. Did I miss something
You're right, though my comment remains the same. When you look under the source code of .NET regarding the TryParse method of Guid, you see that there's a lot being done before going under a try / catch clause, suggesting that Microsoft found it might be possible this part could crash under certain scenarios.
My point is, under Identity.TryParse, there's clearly a way for Optimizely to parse the string without even try catching at all. So, instead of calling "new Identity(string)", the TryParse method could do the following instead:
Hopes it helps understand my concern.
the part you mentioned is actually done in new Identity(string) (there is a try catch throw there). so IMO it's good enough. Yes there could be some improvements but nothing major