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

Compare versions properly #126

Merged
merged 2 commits into from
Dec 9, 2021
Merged

Compare versions properly #126

merged 2 commits into from
Dec 9, 2021

Conversation

Trenly
Copy link
Owner

@Trenly Trenly commented Dec 1, 2021

@Trenly
Copy link
Owner Author

Trenly commented Dec 1, 2021

@OfficialEsco If you want me to rebase these on the latest to make testing easier LMK

@OfficialEsco
Copy link
Collaborator

OfficialEsco commented Dec 1, 2021

Naah i cherry picked every branch into 1 on my repo ^^ Just doing testing under normal usage which is why some PR's gets approved slowly as they require a bit more stable testing

Edit: not sure how much you test your code, but if you do you'll probably pick a few that is mentioned, which doesn't always cover every corner (:

Edit2: I daily drive every patch, however i'm not sure what the state of #112 is, also its probably a bit too early to use since we don't have all the areas covered (MS Side)

@Trenly
Copy link
Owner Author

Trenly commented Dec 1, 2021

Naah i cherry picked every branch into 1 on my repo ^^ Just doing testing under normal usage which is why some PR's gets approved slowly as they require a bit more stable testing

Fair point; Just didn't know if it was easy for you

Edit: not sure how much you test your code, but if you do you'll probably pick a few that is mentioned, which doesn't always cover every corner (:

Depends on the case. If the issue is clear and easy to solve like the architecture bug, I do a little less testing. For more complex things, I do a little more testing.

Edit2: I daily drive every patch, however i'm not sure what the state of #112 is, also its probably a bit too early to use since we don't have all the areas covered (MS Side)

I'm thinking of waiting on #112 for a bit. I know there is a bug open for release date validation. I'm going to mark #112 as draft probably, just since it isn't critical to be merged in

@OfficialEsco
Copy link
Collaborator

OfficialEsco commented Dec 2, 2021

Well, here is one issue microsoft#36724
Also looks like i missed this one microsoft#36633
Pretty sure this one also crashed microsoft#36642

Wait, are 2 of those intentional?

if ($AllVersions.Count -eq '1') { $CommitType = 'New package' }

@Trenly
Copy link
Owner Author

Trenly commented Dec 2, 2021

2 of those are intentional; Yes

Can you describe the issuse with each of those three packages in a bit more detail?

@OfficialEsco
Copy link
Collaborator

Nevermind, all of those are actually a trigger of 1 version in the repo

  • Guilded: Dynamic url, AutoUpgrade
  • SubSync: there was 1 version in there, Simple Update
  • Sandboxie.Plus: I submitted 0.9.8d first, dragged those files into the folder and SimpleUpgraded to 1.0.0.0.0 so i expected New version: instead of New package:

The .Count line should probably be last in the elseif's or rewritten so it dosn't trigger when there already is a version in there. (Because i regularly update Dynamic urls and it looks weird when it always says New package:)

@Trenly
Copy link
Owner Author

Trenly commented Dec 2, 2021

Nevermind, all of those are actually a trigger of 1 version in the repo

  • Guilded: Dynamic url, AutoUpgrade
  • SubSync: there was 1 version in there, Simple Update
  • Sandboxie.Plus: I submitted 0.9.8d first, dragged those files into the folder and SimpleUpgraded to 1.0.0.0.0 so i expected New version: instead of New package:

The .Count line should probably be last in the elseif's or rewritten so it dosn't trigger when there already is a version in there. (Because i regularly update Dynamic urls and it looks weird when it always says New package:)

Ahh, fair point. I’ll take a look at those cases and see where the logic needs to change

@Trenly
Copy link
Owner Author

Trenly commented Dec 6, 2021

  • Guilded: Dynamic url, AutoUpgrade

I believe this is fixed

  • SubSync: there was 1 version in there, Simple Update

I believe this is fixed

  • Sandboxie.Plus: I submitted 0.9.8d first, dragged those files into the folder and SimpleUpgraded to 1.0.0.0.0 so i expected New version: instead of New package:

Not entirely sure what happened here. I wasn't able to replicate. If it is still happening, maybe we can chat on Gitter to figure it out?

The .Count line should probably be last in the elseif's or rewritten so it dosn't trigger when there already is a version in there. (Because i regularly update Dynamic urls and it looks weird when it always says New package:)

This actually was the root cause of both issues that were resolved. If there was only one version of the package, it was automatically casting the variables as strings. I explicitly cast them as arrays, which should have fixed the issue

Copy link
Collaborator

@OfficialEsco OfficialEsco left a comment

Choose a reason for hiding this comment

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

I've not had too many weird packages, but as far as i can tell this is working as expected

@Trenly Trenly merged commit ff9df04 into v2.1.0 Dec 9, 2021
@Trenly Trenly deleted the FixVersionCompare branch December 9, 2021 22:19
Trenly added a commit that referenced this pull request Dec 10, 2021
* Compare versions properly

* Properly cast to array when only single installer
* Bump Version
denelon pushed a commit to microsoft/winget-pkgs that referenced this pull request Dec 10, 2021
* Compare versions properly (#126)

* Compare versions properly

* Properly cast to array when only single installer
* Bump Version

* Make items lowercase and remove whitespaces (#127)
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.

[New Feature]: Neutral/Better if newer version
2 participants