microsoft / winget-create

The Windows Package Manager Manifest Creator command-line tool (aka wingetcreate)
MIT License
489 stars 84 forks source link

Shift common installer fields to manifest root level #409

Closed mdanish-kh closed 1 year ago

mdanish-kh commented 1 year ago

This change modifies the Update flow (both interactive and autonomous) to shift the manifest root values to installer level at the start of the update for easier program logic and then shifting the common installer values back to manifest root at the end of the update for better human readable manifests. Similar shifting done at the end of New Command flow as well.

Broke PromptPropertiesAndDisplayManifests into multiple functions for better separation of concerns and so that shifting can be done before the call to DisplayManifestPreview()

Manually verified with packages like DevHome, Terminal, PowerToys, WinGetCreate to make sure I wasn’t breaking any existing CIs with this change.

Also refactored previous function for consistent naming

Edit after merge: This should also fix #211 since the root values are now moved and overwritten to the installer level at the start of the update flow which will make it so that the ProductCode always correctly gets updated and then moved to the root level if it's common between all installers.


ryfu-msft commented 1 year ago

/azp run

azure-pipelines[bot] commented 1 year ago
Azure Pipelines successfully started running 1 pipeline(s).
mdanish-kh commented 1 year ago

Took me some time to get around how the tests are added. While I was adding the test case I realized that the properties that are partial classes were not getting compared correctly and we needed to add the .Equals() override for those properties. Also had to redundantly move the methods to autonomous and interactive update methods so that the results are visible in test cases. Updated existing test cases to match the new behavior introduced by this PR.

ryfu-msft commented 1 year ago

/azp run

azure-pipelines[bot] commented 1 year ago
Azure Pipelines successfully started running 1 pipeline(s).
mdanish-kh commented 1 year ago

Let me know if I messed up somewhere while addressing the review comments 😄VS is warning me to implement GetHashCode() overrides and separate each partial class into a separate file. Let me know if this is something I should look into or something that we can ignore for now.

ryfu-msft commented 1 year ago

/azp run

azure-pipelines[bot] commented 1 year ago

No commit pushedDate could be found for PR 409 in repo microsoft/winget-create

ryfu-msft commented 1 year ago

/azp run

azure-pipelines[bot] commented 1 year ago
Azure Pipelines successfully started running 1 pipeline(s).
ryfu-msft commented 1 year ago

/azp run

azure-pipelines[bot] commented 1 year ago
Azure Pipelines successfully started running 1 pipeline(s).