Skip to content

Support floating prerelease and stable packages at the same time with the same version range (enable absolute latest scenario) (*-*)#3247

Merged
nkolev92 merged 13 commits intodevfrom
dev-nkolev92-w
Mar 17, 2020
Merged

Support floating prerelease and stable packages at the same time with the same version range (enable absolute latest scenario) (*-*)#3247
nkolev92 merged 13 commits intodevfrom
dev-nkolev92-w

Conversation

@nkolev92
Copy link
Member

@nkolev92 nkolev92 commented Feb 19, 2020

Bug

Fixes: NuGet/Home#912
Regression: No

  • Last working version:
  • How are we preventing it in future:

Fix

Details: Spec at https://github.com/NuGet/Home/blob/dev/designs/FloatingStableAndPrerelease.md
You can test out the implementation at the following link

Testing/Validation

Tests Added: Yes
Reason for not adding tests:
Validation: Lots of unit tests, covering all scenarios.

@nkolev92 nkolev92 self-assigned this Feb 19, 2020
@nkolev92 nkolev92 mentioned this pull request Feb 20, 2020
Closed
awesley
awesley reviewed Feb 24, 2020
View reviewed changes
Copy link
Contributor

@awesley awesley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🍕

@nkolev92
Copy link
Member Author

nkolev92 commented Feb 27, 2020

Thanks for the review @awesley. :)

cristinamanum
cristinamanum reviewed Mar 9, 2020
View reviewed changes
test/NuGet.Core.Tests/NuGet.Versioning.Test/FloatingRangeTests.cs Outdated
var valid = FloatRange.TryParse(floatVersionString, out range);

// Assert
Assert.True(valid);
Copy link
Contributor

@cristinamanum cristinamanum Mar 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can range verification added in the test as well?

Copy link
Member Author

@nkolev92 nkolev92 Mar 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you clarify what specifically you are referring to?

cristinamanum
cristinamanum reviewed Mar 9, 2020
View reviewed changes
donnie-msft
donnie-msft reviewed Mar 11, 2020
View reviewed changes
@nkolev92 nkolev92 requested a review from donnie-msft March 11, 2020 23:01
donnie-msft
donnie-msft requested changes Mar 12, 2020
View reviewed changes
@nkolev92 nkolev92 requested a review from donnie-msft March 12, 2020 01:15
@nkolev92
Copy link
Member Author

nkolev92 commented Mar 12, 2020

@donnie-msft Addressed feedback in the latest commit.
Resolved the accepted comments, replied to the other 2.

donnie-msft
donnie-msft requested changes Mar 12, 2020
View reviewed changes
Copy link
Contributor

@donnie-msft donnie-msft left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@donnie-msft Addressed feedback in the latest commit.
Resolved the accepted comments, replied to the other 2.

Thanks. Looks good, but I'll let someone else approve to overrule on the coding guideline.

@nkolev92
Copy link
Member Author

nkolev92 commented Mar 12, 2020

I have change the test names and moved them around in better classes as per what I suggested in the TODO that @donnie-msft commented on #3247 (comment).

PTAL.

@nkolev92 nkolev92 mentioned this pull request Mar 12, 2020
Merged
dominoFire
dominoFire reviewed Mar 16, 2020
View reviewed changes
Copy link
Contributor

@dominoFire dominoFire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Just style and variable assigments comments.

@nkolev92 nkolev92 requested a review from dominoFire March 16, 2020 19:06
@nkolev92
Copy link
Member Author

nkolev92 commented Mar 16, 2020

I broke my tests somehow :(

@nkolev92
Copy link
Member Author

nkolev92 commented Mar 17, 2020

I had an off by 2 error.

Will merge once tests are green.

@nkolev92 nkolev92 merged commit 69458fe into dev Mar 17, 2020
@nkolev92 nkolev92 deleted the dev-nkolev92-w branch March 17, 2020 08:02
@rrelyea rrelyea changed the title Support floating prerelease and stable packages at the same time with the same version range (enable absolute latest scenario) Support floating prerelease and stable packages at the same time with the same version range (enable absolute latest scenario) (*-*) Apr 2, 2020
@nkolev92 nkolev92 mentioned this pull request May 23, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@donnie-msft donnie-msft donnie-msft approved these changes

@aortiz-msft aortiz-msft Awaiting requested review from aortiz-msft

@dtivel dtivel Awaiting requested review from dtivel

@heng-liu heng-liu Awaiting requested review from heng-liu

@kartheekp-ms kartheekp-ms Awaiting requested review from kartheekp-ms

@rrelyea rrelyea Awaiting requested review from rrelyea

@zivkan zivkan Awaiting requested review from zivkan

@zkat zkat Awaiting requested review from zkat

+3 more reviewers

@awesley awesley awesley left review comments

@dominoFire dominoFire dominoFire approved these changes

@cristinamanum cristinamanum cristinamanum approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@nkolev92 nkolev92

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Support pre-release packages with floating versions. Version="*-*" and similar.

6 participants

@nkolev92 @dominoFire @awesley @zivkan @cristinamanum @donnie-msft