-
Notifications
You must be signed in to change notification settings - Fork 694
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
Semver 2.0 support for VS 2015 #1572
Conversation
This change adds support for selecting v3 services based on the client version.
…protocol), equivalent of PR#1128
Why did we port #1094? It's not necessary for this to work, right? |
@joelverhagen |
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.
RegistrationBaseUrl/3.6.0
is not in PROD yet. Please let me know when these details are finalized so we can get it on PROD
.
@joelverhagen perfect, thanks. Will ping you when ready. |
Retargetted the branch. |
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.
Small comments. But Good Job otherwise! 🚢
private readonly DateTime _requestTime; | ||
private List<Uri> _empty; | ||
private static readonly IReadOnlyList<ServiceIndexEntry> _emptyEntries = new List<ServiceIndexEntry>(); |
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.
suggestion: Some comments on each of these would be nice to have. Else it is harder to understand.
foreach (var type in orderedTypes) | ||
{ | ||
List<ServiceIndexEntry> entries; | ||
if (_index.TryGetValue(type, out entries)) |
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.
not: List<ServiceIndexEntry> entries
do not need a separate declaration. they can be part of the function call. I can give you an example if needed.
#region PSAutoCompleteV3Example | ||
public const string PsAutoCompleteV3Example = @"{ | ||
public const string AutoCompleteV2Example = @"[ | ||
""elm.TypeScript.DefinitelyTyped"", |
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.
👍
Resolves NuGet/Home#5560
There are the 3 PRs that were ported:
#1094
#1128
#1325
Fixed the inconsistencies and added some tests for that.
Now NuGet-3.6.0 will hit the "RegistrationBaseUrl/3.6.0" which was added in https://github.com/NuGet/NuGet.Services.Index/pull/70
Note
This PR is ready, but it's just a matter of how we work with the branches/versions.
We'll wait for @rrelyea before pulling the trigger on either this PR and the version changes currently in the base (release-3.5.1-rtm)