Fixes microsoft#21219 + some improvements

[vedantmgoyal9]

• Have you signed the Contributor License Agreement?
• Have you checked that there aren't other open pull requests for the same manifest update/change?
• Have you validated your manifest locally with winget validate --manifest <path>?
• Have you tested your manifest locally with winget install --manifest <path>?
• Does your manifest conform to the 1.0 schema?

Note: <path> is the name of the directory containing the manifest you're submitting.

---

[OfficialEsco]

I'll start testing this now, my last 60+ PRs used version 1 of the SimpleUpdate

[OfficialEsco]

Well, something is broken

Select Mode

[1] New Manifest or Package Version

[2] Quick Update Package Version (Note: Must be used only when previous version's metadata is complete.)

[3] Update Package Metadata

[4] New Locale

[q] Any key to quit

Selection: 2

[Required] Enter the Package Identifier, in the following format <Publisher shortname.Application shortname>. For example: Microsoft.Excel
PackageIdentifier: Bethesda.Launcher

[Required] Enter the version. for example: 1.33.7
Version: 1.82.0
Found Existing Version: 1.75.0
   ~\GitHub\winget-pkgs  PR-Parms                                                                    10:41:46 
❯

[vedantmgoyal9]

@OfficialEsco I have fixed the issue.

@Trenly If you do any changes to the script, please commit directly to my branch. It is very difficult for me to update the branch and resolve conflicts.

[Trenly]

@OfficialEsco I have fixed the issue.

@Trenly If you do any changes to the script, please commit directly to my branch. It is very difficult for me to update the branch and resolve conflicts.

Rebasing can be extremely difficult. It is a good skill to learn though, is how to read through and resolve merge conflicts during a rebase. I'll try to avoid any big changes while you have open PR's. I also don't want us to get into a situation where we have a branch of a branch of a branch of a branch for creating and merging in new features though, so I can't always guarantee there won't be conflicts

[Trenly]

Sorry I missed these!

[vedantmgoyal9]

@Trenly I committed all suggestions but then read your message.

So, if you comment the line, you will find that if I try to create a non-existing (new) package, it proceeds to enter InstallerUrl and the rest of the data. But, if you uncomment that line, and the package does not exist before...

the script will throw error and exit.

The option is meant for only update the existing package version, so it is must that package should exist before selecting that option.

[vedantmgoyal9]

#23 (comment) @Trenly you can only do. I have tried again but my brain got fully drained 🤯.

[Trenly]

@Trenly I committed all suggestions but then read your message.
So, if you comment the line, you will find that if I try to create a non-existing (new) package, it proceeds to enter InstallerUrl and the rest of the data. But, if you uncomment that line, and the package does not exist before...
the script will throw error and exit.

The option is meant for only update the existing package version, so it is must that package should exist before selecting that option.

Ohhh, I see. So it needs to be in both places. I misunderstood

[Trenly]

#23 (comment) @Trenly you can only do. I have tried again but my brain got fully drained 🤯.

What exactly isn't working about the multi-manifest Simple Update?

[vedantmgoyal9]

Esco wants more fewer prompts. #23 (comment)

Edit: Original Issue: microsoft#21219

[vedantmgoyal9]

@OfficialEsco @Trenly I have another feature that can be implemented pretty easily.

While creating manifests for packages like Mirantis.Lens, Fndroid.ClashForWindows, etc. we have to enter one URL two times to create different installers due to different scope/installer switches.

So, instead of downloading the same URL two times, it should check if that URL was entered before and if the condition is true, get the previous hash and continue to the next field.

[Trenly]

@OfficialEsco @Trenly I have another feature that can be implemented pretty easily.

While creating manifests for packages like Mirantis.Lens, Fndroid.ClashForWindows, etc. we have to enter one URL two times to create different installers due to different scope/installer switches.

So, instead of downloading the same URL two times, it should check if that URL was entered before and if the condition is true, get the previous hash and continue to the next field.

Sounds like a good idea; Lets make a separate issue / pr for it when we do implement it

[Trenly]

Esco wants more fewer prompts. #23 (comment)

Edit: Original Issue: microsoft#21219

@OfficialEsco @vedantmgoyal2009 I've created a PR over at Vedant's repo to make the prompts reduced the way Esco had in the original post

https://github.com/vedantmgoyal2009/winget-pkgs/pull/4

[vedantmgoyal9]

I think the warning should be displayed before entering PackageIdentifier because if a user sees a warning and then exits and starts over, he will have enter pkg id and version again. Also, is there any way to switch the mode without exiting the script?

[Trenly]

I think the warning should be displayed before entering PackageIdentifier because if a user sees a warning and then exits and starts over, he will have enter pkg id and version again. Also, is there any way to switch the mode without exiting the script?

We can move the warning, that shouldn’t be too difficult;

I can look into switching the option when I move the warning

[vedantmgoyal9]

Oh, I have done it already. Pushing a new commit in a few minutes.

[Trenly]

Oh, I have done it already. Pushing a new commit in a few minutes.

Still pushing that commit, or want me to run one through quick?

[vedantmgoyal9]

I was correcting a spelling mistake. It took me an hour to find it.

[vedantmgoyal9]

And, in "another feature" I think we have to submit two PR, first of deleting the old version and then adding a new version.

No it does not because the URL is static

Read Previous Manifest
Create New Manifest
Delete old Manifest if InstallerURL is the same
Push Commit
Create PR

Or

Read Previous Manifest
Create New Manifest
Push Commit
Check if InstallerURL matches previous version
If match Delete old Manifest
Push Commit
Create PR

We have to do this part also?

Edit: I just now realised that does @wingetbot do the same thing while it is creating "Automatic Update" PRs?

[vedantmgoyal9]

Can we move code from functions like Test-Manifest, Submit-Manifest, Read-WinGet-MandatoryInfo, Read-PreviousWinGet-Manifest-Yaml, Show-OptionMenu at the last as you have done in commit 1f614b5

[Trenly]

We have to do this part also?

Edit: I just now realised that does @wingetbot do the same thing while it is creating "Automatic Update" PRs?

We will have to add in the functionality to automatically delete old versions, yes, but we don't want to stack too many features into a single PR. The reason for this is that it makes any single change less likely to break the code. It's much easier to roll back a single feature to undo a mistake than it is to roll back all the features in a large PR and figure out which one caused the error. A rule of thumb I like to follow is to have one issue per PR unless they are extremely closely related.

I did create #32 so we can track working on the deletion of previous manifests; I don't believe it is necessary to complete to merge the quick updates in though, since Esco has it mentioned as an "additional feature" in the original request

[Trenly]

Can we move code from functions like Test-Manifest, Submit-Manifest, Read-WinGet-MandatoryInfo, Read-PreviousWinGet-Manifest-Yaml, Show-OptionMenu at the last as you have done in commit 1f614b5

Yes, we can move code outside of functions if it makes sense to do so; However, if you do this you have to be very careful to respect program flow. More than once I've moved something outside of a function thinking it would be fine, only to find out later that the program flow wasn't quite the way I thought it was.

There are a few reasons that we would also want to use functions; 1) Repeated code; The test and submit functions are common to all of the options, so it makes the most sense to have them as a function; 2) Readability; Even if you can do something without a function, sometimes having a named function makes it easier for others to understand the intent of the code

Tl;dr - Yes, in some cases, we just should be careful about it

[vedantmgoyal9]

@Trenly Quick Update does not take Installer Switches from previous manifest.

Edit: It takes Installer Switches but why winget displays a warning 🤔?

[Trenly]

@Trenly Quick Update does not take Installer Switches from previous manifest.
Edit: It takes Installer Switches but why winget displays a warning 🤔?

It is because winget sees they are empty strings; Even if the values are defined, if the strings are empty then they are handled as if they are not specified

[Trenly]

@OfficialEsco Some pretty big restructures in this one; If you have any issues with it, please let us know. I'd like to merge this in before I start working on #30

[OfficialEsco]

It actually feels like this is ready to go tbh

[Trenly]

It actually feels like this is ready to go tbh
Sweet! I'll merge it in soon then