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

(#3174) Apply version number normalizing consistently #3175

Merged
merged 1 commit into from
Jun 5, 2023

Conversation

vexx32
Copy link
Member

@vexx32 vexx32 commented May 31, 2023

Description Of Changes

Went through all references to ToStringSafe which were being called on a NuGetVersion instance and changed them all to ToNormalizedStringChecked.

Additionally, found a place in the upgrade dry-run code that was just missing a call to this method and outputting a non-normalized version string and added it.

Motivation and Context

We should be consistent in how we internally and externally represent versions as strings, so that folks relying on our output for consistent comparisons don't need to think too hard about it.

Testing

  1. choco install 7zip --ignore-dependencies
  2. choco upgrade --noop 7zip -r
  3. The output result should display 7zip|22.1.0|22.1.0|false exactly (unless a new version of 7zip has been installed, so just verify the version numbers match exactly)

We might be able to add a smoke test package for this case, but I've not had the time to look into it as yet. I will update here if I manage to get that done as well.

Operating Systems Testing

WIn11

Change Types Made

  • Bug fix (non-breaking change).
  • Feature / Enhancement (non-breaking change).
  • Breaking change (fix or feature that could cause existing functionality to change).
  • Documentation changes.
  • PowerShell code changes.

Change Checklist

  • Requires a change to the documentation.
  • Documentation has been updated.
  • Tests to cover my changes, have been added.
  • All new and existing tests passed?
  • PowerShell code changes: PowerShell v2 compatibility checked?

Related Issue

Fixes #3174

@vexx32 vexx32 requested a review from AdmiringWorm May 31, 2023 19:07
@vexx32 vexx32 marked this pull request as draft May 31, 2023 21:00
Copy link
Member

@AdmiringWorm AdmiringWorm left a comment

Choose a reason for hiding this comment

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

In addition to the requested changes, the failing tests need to be investigated as well.

@AdmiringWorm AdmiringWorm force-pushed the normalize-versions-everywhere branch 2 times, most recently from 723611a to e9e4638 Compare June 1, 2023 15:49
@AdmiringWorm AdmiringWorm marked this pull request as ready for review June 1, 2023 16:09
@AdmiringWorm AdmiringWorm requested a review from gep13 June 1, 2023 16:09
@AdmiringWorm AdmiringWorm dismissed their stale review June 1, 2023 16:09

Changes made by person, needs to be reviewed by someone else.

Verified

This commit was signed with the committer’s verified signature. The key has expired.
domdfcoding Dominic Davis-Foster
Went through all references to `ToStringSafe` which were being called on
a NuGetVersion instance and changed them all to
`ToNormalizedStringChecked`.

Additionally, found a place in the upgrade dry-run code that was just
missing a call to this method and outputting a non-normalized version
string and added it.
@gep13 gep13 force-pushed the normalize-versions-everywhere branch from e9e4638 to f52fc14 Compare June 5, 2023 14:38
Copy link
Member

@gep13 gep13 left a comment

Choose a reason for hiding this comment

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

LGTM!

@gep13 gep13 merged commit e0939de into chocolatey:develop Jun 5, 2023
@gep13
Copy link
Member

gep13 commented Jun 5, 2023

@AdmiringWorm @vexx32 thanks for getting this updated!

@vexx32 vexx32 deleted the normalize-versions-everywhere branch July 3, 2023 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Package version normalization is not being applied everywhere
3 participants