Fix up null check in Timestamps.TryParse

[captainsafia]

Spotted this while working my way through TODOs in the codebase.

I think the right approach is to return false and set the result to default if the input is null. AFAIK, this is in line with null-handling behavior in TryParse methods in the BCL.

Is there a particular reason we threw an exception here previously? Happy to adjust the PR as needed.

[jskeet]

While it's been a bit of time since I wrote this, I believe it was deliberate. This is effectively an internal method (because it's in an internal class) and for the use cases we have, we shouldn't be calling the method with null input - that would indicate something else being wrong in our code to start with. (Someone passing in an event with an invalid timestamp is their fault; us passing in null because they haven't specified a timestamp at all is our fault.)

It's only actually called by tests and by Timestamps.Parse - I do think it's better for a non-nullable string parameter to end up with an ArgumentNullException than some other silent behavior. (Note that our parameter is non-nullable, vs the nullable parameter in DateTimeOffset.TryParse for example.)

We should definitely add a test and better documentation, and probably make the method internal (or even potentially private) - but I want to keep the distinction between null input and "an invalid timestamp string".

[captainsafia]

While it's been a bit of time since I wrote this, I believe it was deliberate. This is effectively an internal method (because it's in an internal class) and for the use cases we have, we shouldn't be calling the method with null input - that would indicate something else being wrong in our code to start with. (Someone passing in an event with an invalid timestamp is their fault; us passing in null because they haven't specified a timestamp at all is our fault.)

Thanks for the explainer here. It tracks to me that the requirements for what is effectively a pubternal API will be different.

We should definitely add a test and better documentation, and probably make the method internal (or even potentially private) - but I want to keep the distinction between null input and "an invalid timestamp string".

I'd plus-one to making this method internal (or private if possible) since it makes a little easier to understand why it has a non-standard contract compared to other TryParse methods.

Alright! I'll push another commit to this branch that:

• Changes the code to throw ANE for null inputs
• Adds a test to validate that specific behavior
• Changes the visibility of the methods

[jskeet]

Thanks!