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

Replaced PrBodyContentReply `n with array #12

Merged

Conversation

OfficialEsco
Copy link
Collaborator

I just learned the use of @() 🤦‍♂️
Needed the space on line 983 to make the PR look less cluttered

@OfficialEsco OfficialEsco requested a review from Trenly August 12, 2021 11:36
@Trenly
Copy link
Owner

Trenly commented Aug 12, 2021

@OfficialEsco I’m wondering if we can abstract the switch a bit further now that we have the standardized menus.

Soemthing like:

Function
  For each line
    Switch -Wildcard
        Set the menu options, don’t display menu
    End switch
    Display menu and replace line
  Next line
End function

The more I think about it though, the more I believe the KeypressMenu function would need to be adjusted to handle empty strings. I can handle this in a separate PR

@OfficialEsco
Copy link
Collaborator Author

OfficialEsco commented Aug 12, 2021

Think its best if you just show it in action, but usually your functions make a lot more sense 😁

Copy link
Owner

@Trenly Trenly left a comment

Choose a reason for hiding this comment

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

All comments are just minor suggestions and could be handled in a separate PR

Foreach ($i in ($ResolvedIssues.Split(",").Trim())) {
$PrBodyContentReply += "Resolves #$i`n"
Copy link
Owner

Choose a reason for hiding this comment

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

I know its a bit outside the scope of your changes, but is there any way for us to validate the issue exists? Maybe using Invoke-WebRequest and checking the return status code?

Also, I know this isn’t your fault, but the way the issues are handled makes it so if someone entered microsoft/winget-cli/#1234 or #1234 it wouldn’t properly link the resolves. I think we need to have separate logic handling for if the $i has a # in it already

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was actually thinking the same thing, not sure how well the # matching is, but technically it can match towards a old PR or Issue which is not supposed to be closed, the tips does however say to use 123, 1234 instead of #123 or microsoft/winget-cli/#1234 tho

You can probably make it safer in another PR since you sorta already have a plan and know what you are doing 😁
Ill just jump in with my suggestions and stress test it 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Off topic: i have/had a issue when fetching some patterns from the Schema, for example i would like the Pattern from PackageVersion which is ^[^\\\\/:\\*\\?\"<>\\|\\x01-\\x1f]+$ however the output turned into something like ^[^\/:\*\?\"<>\|\x01-\x1f]+$ which obviously did not work

Do you have a workaround for this?

Copy link
Owner

Choose a reason for hiding this comment

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

Do you have specific line numbers that I can look at?

Copy link
Collaborator Author

@OfficialEsco OfficialEsco Aug 12, 2021

Choose a reason for hiding this comment

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

Line 176 i want
-and ($PackageVersion -notmatch $InstallerSchema.definitions.PackageVersion.pattern)

You can test this

$InstallerSchema = @(Invoke-WebRequest 'https://raw.githubusercontent.com/microsoft/winget-cli/master/schemas/JSON/manifests/v1.0.0/manifest.installer.1.0.0.json' -UseBasicParsing | ConvertFrom-Json)
$InstallerSchema.definitions.PackageVersion.pattern

Output: ^[^\\/:\*\?"<>\|\x01-\x1f]+$ expected ^[^\\\\/:\\*\\?\"<>\\|\\x01-\\x1f]+$

@Trenly Trenly merged commit 097e2c7 into Trenly:YamlParsing Aug 12, 2021
@OfficialEsco OfficialEsco deleted the PrBodyContentReply-Array-instea-of-`n branch August 12, 2021 13:18
Trenly pushed a commit that referenced this pull request Aug 20, 2021
Trenly pushed a commit that referenced this pull request Sep 14, 2021
Trenly pushed a commit that referenced this pull request Sep 27, 2021
Trenly pushed a commit that referenced this pull request Oct 13, 2021
Trenly added a commit that referenced this pull request Nov 3, 2021
* Install powershell-yaml

* Enforce ordering of keys

* Create function for adding list parameters

* Write Version Manifest using Yaml Parser

* Add To-Do Section

* Rebase on master and cleanup commit history

* Revert inadvertent changes from testing

* Implement logic for installers using YAML parser

* Fix statement in incorrect position

* Remove RAW writing

* Begin reading MultiManifests using Yaml parser

* Cleanup unneeded variables

* Bump Version

* Read parameters from singletons

* Add Product Code

* Abstract ReadInstallerManifest to sub-function

* Fix: Handle uninitialized variables

* Merge New/Update, add EditMetadata

* Fix: Change EditMetadata to work on singleton manifests

* Fix: Update Git commit messages for accuracy

* Remove Empty Manifest Folders

* Fix: Correct text

* Fix: Remove code for testing

* Feature: If old manifests exist, update in place to preserve extra keys

* Fix: Null Error when creating new

* Fix: Cast only when installer code

* Fix: Respect installer locale

* Fix: Use UTF8 Encoding

* Sort Yaml Keys

* Git error handling

* Fixed Moniker prompt for NewLocale

* minItems, maxItems, pattern, variable fix

* Implemented minItems from Schema
* Implemented maxItems from Schema
* Implemented pattern from Schema
* Implemented available Architecture's from Schema
* Implemented available InstallerType's from Schema
* Added $script: to some variables which needs to stay throughout the script

Only tested with one manifest as of now

Cleanup If/Else logic & Rebase on master

* Update method of getting Product Code from installer

* SignatureSha256 and PackageFamilyName (#5)

* More CommitTypes

* Fill in PR Parameters microsoft#21940 (#4)

* Detect PackageFamilyName for MSIX/APPX (#8)

* Update YamlCreate.ps1

* Update YamlCreate.ps1

* Exclude .validation (#9)

* Exclude .validation

* Added exclude to ExistingVersions

Co-authored-by: Levvie - she/her <[email protected]>

* Add new line for PackageFamilyName (#10)

* Standardize Keypress Menus (#11)

* Standardize Keypress Menus

* Fix: Use variable instead of pipeline

* Fix Help Texts & Spacing

* Cleanup Git Messages

* Filter by Yaml

* Remove debugging line

Co-authored-by: Esco <[email protected]>

Co-authored-by: Kaleb Luedtke <[email protected]>
Co-authored-by: Esco <[email protected]>

* Replaced PrBodyContentReply `n with array (#12)

* Menu reduction (#13)

* Simplify Code + Make Issues Entry Safer

* Check issue validity

* Fix unintended Revert

Co-authored-by: Esco <[email protected]>

* Fix unintended Revert

Co-authored-by: Esco <[email protected]>

* Fix unintended Revert

Co-authored-by: Esco <[email protected]>

Co-authored-by: Esco <[email protected]>

* More patterns (#15)

Added pattern for

* PackageIdentifier
* PackageVersion
* PackageFamilyName
* PackageLocale

* Readability (#16)

* Add Comparison Functions for Readability

* Simplify Validation Function

* Fix: Missing text

* Remove spaces from split function

* Fix: Resolve renamed variable

* Validate Installer Modes

* Validate File Extensions with Pattern and Length

* Add Custom Error Class (#18)

* Create function for adding list parameters

* Remove RAW writing

* Remove Empty Manifest Folders

* minItems, maxItems, pattern, variable fix

* Implemented minItems from Schema
* Implemented maxItems from Schema
* Implemented pattern from Schema
* Implemented available Architecture's from Schema
* Implemented available InstallerType's from Schema
* Added $script: to some variables which needs to stay throughout the script

* Add Custom Error Class

* Cleanup Class Constructor

* Additional error messages + cleanup

* Continue adding error messages

* Switched Local formatter to OTBS + Static Strings (Where applicable)

* Fix Spacing

* Remove unused variables from rebase

Co-authored-by: Esco <[email protected]>

* Fix: Check for package before do-until

* Finish Locale Errors

* Fix Spacing Issues

* Make Enums Case Sensitive

* Fix Unique Items + Incorrect Variables

Co-authored-by: Esco <[email protected]>

* Subfolder fix (#21)

* Fix: Subfolder Exists Erroring

* Return

* Write Unused Keys as Comments (#22)

* Write Unused Keys as Comments

* Exclude certain keys from appearing as comments

* Comment only Locale + Version

* Quick Update, Function Reduction, Detect Installer Types, Detect Installer Architectures, and Comments (#27)

* Update YamlCreate.ps1

* auto-detect installer architecture

* Fix #27 (comment)

* Fix auto-detection

* Apply suggestions from code review

Co-authored-by: Kaleb Luedtke <[email protected]>

* Move Test + Submit to main

* Update Tools/YamlCreate.ps1

* fix quick-update check

* Reduce input required for quick version update (#4)

* Reduce input required for quick version update

* Fix Product Code

* Don't add null product code if no product code key exists

* improve quick update

* Move prompt outside of Read-Info for clarity

* Move Update Prompt outside of Read-Info for clarity

* Update Messages

* if there are any errors due to this commit, i will revert changes

* Add Comments for code clarity

Co-authored-by: Kaleb Luedtke <[email protected]>

* Fix error from when functions were moved

* Handle Manifest Level Parameters (#34)

* Begin Handling Manifest Level Parameters

* Fix InstallModes

* Add delete manifest functionality (#39)

* Add functionality to manually delete manifests

* Add settings file and script documentation (#38)

* Do not default installer locale

* Add a script settings file with documentation

* Add setting to suppress quick update warning

* Remove accidental file inclusion

* Fix: Update SignatureSha256 in quick update mode (#41)

* Change Settings to allow for negative suppression (#46)

* Change Settings to allow for negative suppression

* Fix sample settings file

* Invalid SandboxTest variable

Co-authored-by: Vedant Mohan Goyal <[email protected]>

Co-authored-by: Kaleb Luedtke <[email protected]>
Co-authored-by: Esco <[email protected]>
Co-authored-by: Vedant Mohan Goyal <[email protected]>

* Cleaner references to linked issues (#49)

* Command Line Arguments (#50)

* Add switch for settings

* Allow PackageIdentifier and PackageVersion to be passed as optional parameters

* Add -help switch and documentation

* Settings File Location

* Fix spacing issue [#49] #51

Co-authored-by: Kaleb Luedtke <[email protected]>
Co-authored-by: Vedant Mohan Goyal <[email protected]>

* Fix Parameter Reading and Condensing(#54)

* Fix: Allow InstallerSwitch keys to be split between installer level and manifest level on a per-key basis

* Fix: Save parameters to variables before removing them from manifest level

* Fix: Sandboxtest not working

* Fix: Prompt for PFN if file does not exist (#55)

* Auto Mode (#58)

* Auto Mode

* Fix for same version

* Keep ProductCodes for .exe files

* Keep ProductCodes for .exe files in Option-2

* Simplify function names, add debug info (#62)

* Fix: Allow autoupdate to update old package versions (#66)

* Make Simple Update automatically detect Sha256, SignatureSha256, and ProductCode without prompts (#61)

* Make Simple Update automatically detect Sha256, SignatureSha256, and ProductCode without prompts

* Add Parameter Mode (#64)

* Add Parameter Mode

* Update YamlCreate.ps1

* Update YamlCreate.md

Co-authored-by: Vedant Mohan Goyal <[email protected]>

* Use Unique Branch Names (#67)

* Use Unique Branch Names

* Process Branch Names Safely

* Fix: Allow values other than en-US for default locale (#70)

* Made it so git config was only modified for the local repo. (#72)

* Only modify git config for local repo.

* Fixed 2214.

* Support for settings on Linux and macOS (#73)

* Support for settings on Linux and macOS

* Easton's suggestion

* Update YamlCreate.md

* Update Tools/YamlCreate.ps1

Co-authored-by: Kaleb Luedtke <[email protected]>

* Update YamlCreate.ps1

Co-authored-by: Kaleb Luedtke <[email protected]>

* Check for open PR's before submitting (#69)

* Check for open PR's before submitting

* Rebase on c82b39f

* Use API instead of CLI

* Exit when user-choice to terminate

Co-authored-by: Vedant Mohan Goyal <[email protected]>

Co-authored-by: Vedant Mohan Goyal <[email protected]>

* Throw when script error (#74)

* Fix: Add locale when converting from singleton (#75)

* Fix: Branch names when deleting (#79)

* Fix: Don't check for PRs on deletion (#80)

* Update PR Content when removing a manifest (#82)

* Update PR Content when removing a manifest

* More realistic character count limits

* Add is:pr to exclude issues from results (#88)

* Ensure file names are valid before saving (#85)

* Ensure file names are valid before saving

* Fix: Catch when content disposition doesn't exist

* Fix UserAgent not following redirects

* Additional Web Request Parameters

* Fix: Min descriptor length + Agent

* Fix: Escape Regex in variables (#91)

* Fix: Only add moniker to defaultLocale (#92)

* Chore: Add references to YamlCreate documentation (#93)

* Typo (#94)

* Chore: Add note to enable settings

Co-authored-by: denelon <[email protected]>

* Chore: Remove To-do message

* Update YamlCreate.ps1

* Move integer validation (#99)

* Rename String.Validate -> Test-String (#101)

* Suppress Write-Host warnings from ScriptAnalyzer

* Fix missing constructor parameter

* Rename Write-Locale-Manifests -> Write-LocaleManifest

* Rename Write-Version-Manifest -> Write-VersionManifest

* Rename Write-Installer-Manifest -> Write-InstallerManifest

* Rename Read-WinGet-LocaleManifest -> Read-LocaleMetadata

* Rename Read-WinGet-InstallerManifest -> Read-InstallerMetadata

* Rename Read-Installer-Values-Minimal -> Read-QuickInstallerEntry

* Rename Read-Installer-Values -> Read-InstallerEntry

* Rename Enter-PR-Parameters -> Read-PRBody

* Rename Write-Colors -> Write-MulticolorLine

* Use Named Parameters for AddYamlParameter

* Use Named Parameters for AddYamlListParameter

* Use Named Parameters for PromptInstallerManifestValue

* Support ShouldProcess for removing manifests

* Move Downloading of installer to a separate function

* More function name updates

* Fix: Download Method (#107)

* Check for version when setting proxy (#109)

* Use defaultLocale due to microsoft/winget-cli#1646 resolving microsoft/winget-cli#1595

Co-authored-by: Esco <[email protected]>
Co-authored-by: Vedant Mohan Goyal <[email protected]>
Co-authored-by: Levvie - she/her <[email protected]>
Co-authored-by: Kaleb Luedtke <[email protected]>
Co-authored-by: Easton Pillay <[email protected]>
Co-authored-by: denelon <[email protected]>
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.

2 participants