Add netstandard2.0 target

[khellang]

Closes #30

[WalkerCodeRanger]

Thanks for the PR! I think we can probably get some version of this merged, but I want to review it more carefully when I have more time.

Can you tell me more about the changes to the exception message? I'd prefer to have the test assertion cover the whole message not just that it starts with the whole thing.

[khellang]

Can you tell me more about the changes to the exception message?

I can't remember the exact details right now, but basically the exception message has changed in newer versions of .NET (Core). It used to contain a line break, but doesn't anymore. It also has added parentheses around the Parameter name part.

I'd prefer to have the test assertion cover the whole message not just that it starts with the whole thing.

Asserting on complete error messages like this isn't The Best™ as they are not guaranteed to be stable across versions. The remaining Assert.StartsWith still covers the "meat" of the exception message. The only part of the message left is the parameter name part, which is covered by the Assert.Equal("version", ex.ParamName); asserts.

[khellang]

You can see the changes in the message formatting here: .NET Framework and .NET Core. And the PR changing the exception message format: dotnet/coreclr#25185

[khellang]

The lack of this exception was the reason for the following comments in the tests 😅

// TODO that is a strange error message, should be version

[khellang]

Asserting on complete error messages like this isn't The Best™ as they are not guaranteed to be stable across versions. The remaining Assert.StartsWith still covers the "meat" of the exception message. The only part of the message left is the parameter name part, which is covered by the Assert.Equal("version", ex.ParamName); asserts.

@WalkerCodeRanger What do you think? Makes sense? 🤔

[WalkerCodeRanger]

@khellang after having a chance to properly review and look at things, I agree with you that checking the start of the exception message as well as the parameter name is a good solution.

Unfortunately, your PR was done against the master branch while the code for the next version is on the release-2.1 branch. The changes on the release-2.1 are too significant for me to merge this into it. I ended up making my own branch to redo what you did. If you check it out, you'll see I did almost everything exactly as you have done. Your PR was helpful in guiding me and without it, I probably wouldn't have had the impetus to add the .NET Standard 2.0 target yet.

Two things I did differently, I kept the netcoreapp2.1 target for the tests because that is the only one without serialization support. I also didn't add the exception to fix the parameter name. I did that because we are being very conservative in maintaining backward compatibility and v2.1.0 makes the old parsing methods obsolete (for the most part). There are new parsing methods with improved performance, options, and better exceptions. Given that, it reduces the impetus to fix up the old parsing methods.

I am closing this PR since I had to make a new one. The new PR is #67 Add netstandard2.0 to release v2.1.0. I am planning to publish v2.1.0 including this change by the end of March 2022.

[khellang]

Unfortunately, your PR was done against the master branch while the code for the next version is on the release-2.1 branch.

I just opened the PR against your configured main branch on GitHub. I would've gladly rebased my changes on another branch had someone told me 🤷‍♂️

I ended up making my own branch to redo what you did. If you check it out, you'll see I did almost everything exactly as you have done.

IMO, this is really bad form and will make sure I won't consider contributing to this project again 😔

Two things I did differently

You could've just left that feedback and I would've fixed it by tomorrow 😆

I am closing this PR since I had to make a new one.

Again, you didn't have to make a new one. You chose to drop all my work, even though you could've based your PR on top of my work, even maybe squashed my commits first.

I am planning to publish v2.1.0 including this change by the end of March 2022.

I'm just happy there's a release with the new target coming. At the end of the day that's what matters. But the way this PR was handled left a really bad taste I'm my mouth 😣