-
Notifications
You must be signed in to change notification settings - Fork 416
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
Handle package reference version ranges in .csproj files #814
Conversation
@@ -5,21 +5,20 @@ namespace OmniSharp.MSBuild.ProjectFile | |||
{ | |||
public class PackageReference : IEquatable<PackageReference> | |||
{ | |||
public PackageIdentity Identity { get; } | |||
public PackageDependency Identity { get; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe you'd want to go from public PackageIdentity Identity { get; }
to public PackageDependency Dependency { get; }
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes -- thanks!
{ | ||
NuGetVersion version; | ||
if (NuGetVersion.TryParse(propertyValue.Trim(), out version)) | ||
if (VersionRange.TryParse(propertyValue.Trim(), out var version)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe I'm weird but doesn't it strike you as a bit inconsistent that in the same namespace they'd have two different naming styles - NuGetVersion
and VersionRange
(used for, well, NuGetVersions
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a bunch of them: NuGetVersion
, SemanticVersion
, VersionRange
, FloatRange
. They're all in NuGet.Versioning. NuGetVersion
is a specific version, used for package identity. A range wouldn't be suitable for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I did the initial punch through to get package references working in .csproj, I failed to account for version ranges. This should address the unresolved dependencies warnings reported in #795.