Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NuGetVersion.TryParseStrict() should return false if it fails to parse #9255

Closed
VonOgre opened this issue Mar 3, 2020 · 2 comments · Fixed by NuGet/NuGet.Client#3280
Closed
Labels
Functionality:SDK The NuGet client packages published to nuget.org help wanted Considered good issues for community contributions. Type:Bug

Comments

@VonOgre
Copy link

VonOgre commented Mar 3, 2020

Details about Problem

It seems that NuGetVersion.TryParseStrict() doesn't actually ever return false, which isn't ideal for a "Try" method. The implementation actually does check if it can parse to a SemanticVersion, but always returns true anyways...
https://github.com/NuGet/NuGet.Client/blob/bf9523d7b66f749f8709c8f082944db6a60f9153/src/NuGet.Core/NuGet.Versioning/NuGetVersionFactory.cs#L105-L115

NuGet product used (NuGet.exe | VS UI | Package Manager Console | dotnet.exe):
NuGet.Versioning v5.4.0

Detailed repro steps so we can see the same problem

var isValid1 = NuGetVersion.TryParseStrict("NotAVersion", out var parsed1);
// isValid1 == true, parsed == null

var isValid2 = NuGetVersion.TryParseStrict("", out var parsed2);
// isValid2 == true, parsed == null

var isValid3 = NuGetVersion.TryParseStrict("1.0.0", out var parsed3);
// isValid2 == true, parsed != null

@zivkan zivkan added Functionality:SDK The NuGet client packages published to nuget.org Type:Bug help wanted Considered good issues for community contributions. labels Mar 4, 2020
@zivkan
Copy link
Member

zivkan commented Mar 4, 2020

This looks like an easy issue for a community contribution. Tests should be added to test/NuGet.Core.Tests/NuGet.Versioning.Test/NuGetVersionTest.cs. At the moment there are only positive tests, so negative tests need to be added.

@VonOgre
Copy link
Author

VonOgre commented Mar 4, 2020

Indeed, I'd meant to do a fork and PR last night but got sidetracked, but I expect either tonight or tomorrow 👍

@rrelyea rrelyea changed the title NuGet.Versioning NuGetVersion.TryParseStrict() always returns true NuGetVersion.TryParseStrict() should return false if it fails to parse May 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Functionality:SDK The NuGet client packages published to nuget.org help wanted Considered good issues for community contributions. Type:Bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants