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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 13 commits into from
Mar 17, 2020
Merged

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

merged 13 commits into from
Mar 17, 2020

Conversation

nkolev92
Copy link
Member

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.

🍕

src/NuGet.Core/NuGet.Versioning/VersionRangeFactory.cs Outdated Show resolved Hide resolved
src/NuGet.Core/NuGet.Versioning/FloatRange.cs Show resolved Hide resolved
src/NuGet.Core/NuGet.Versioning/FloatRange.cs Outdated Show resolved Hide resolved
@nkolev92
Copy link
Member Author

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

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

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?

@nkolev92 nkolev92 requested a review from donnie-msft March 11, 2020 23:01
@nkolev92 nkolev92 requested a review from donnie-msft March 12, 2020 01:15
@nkolev92
Copy link
Member Author

@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

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.

test/NuGet.Core.Tests/NuGet.Versioning.Test/FloatingRangeTests.cs Outdated Show resolved Hide resolved
src/NuGet.Core/NuGet.Versioning/VersionRange.cs Show resolved Hide resolved
src/NuGet.Core/NuGet.Versioning/FloatRange.cs Outdated Show resolved Hide resolved
@nkolev92 nkolev92 requested a review from dominoFire March 16, 2020 19:06
@nkolev92
Copy link
Member Author

I broke my tests somehow :(

@nkolev92
Copy link
Member Author

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

@awesley awesley awesley left review comments

@dominoFire dominoFire dominoFire approved these changes

@cristinamanum cristinamanum cristinamanum approved these changes

@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

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