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

Bump YamlDotNet to 13.2.0 #433

Merged
merged 5 commits into from
Aug 16, 2023
Merged

Conversation

mdanish-kh
Copy link
Contributor

@mdanish-kh mdanish-kh commented Aug 14, 2023

This change now wraps all string values in the manifest around double quotes single quotes, we can choose from either but I went with one that looks cleaner. Single quotes don't parse escape sequences (not that it matters for our use case i think). (though imo this change makes the overall manifest look ugly now with everything quoted but that's just personal preference 😄)

Tested manually with

wingetcreate update Cockos.REAPER --urls "https://www.reaper.fm/files/6.x/reaper681_x64-install.exe|x64" "https://www.reaper.fm/files/6.x/reaper681-install.exe|x86"
wingetcreate update OpenAL.OpenAL --urls "https://www.openal.org/downloads/oalinst.zip"

Microsoft Reviewers: codeflow:open?pullrequest=https://github.com/microsoft/winget-create/pull/433&drop=dogfoodAlpha

@mdanish-kh mdanish-kh requested a review from a team as a code owner August 14, 2023 07:19
@mdanish-kh mdanish-kh requested review from yao-msft and ryfu-msft and removed request for a team August 14, 2023 07:19
@Trenly
Copy link
Contributor

Trenly commented Aug 14, 2023

(though imo this change makes the overall manifest look ugly now with everything quoted but that's just personal preference 😄)

I agree. Is there a switch to make it quote only values that need to be quoted?

Edit: I see there's quoteNecessaryStrings as part of the event emitter, which does contain Regex for float values. Perhaps we need to set that to true somehow?
https://github.com/aaubry/YamlDotNet/blob/bfba24411c54dee930cf87d93e313f6d724091b0/YamlDotNet/Serialization/EventEmitters/TypeAssigningEventEmitter.cs#L36-L46

Copy link
Contributor

@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.

Found it (suggested above)

image

Co-authored-by: Kaleb Luedtke <trenlymc@gmail.com>
@mdanish-kh
Copy link
Contributor Author

Thanks 🎉 I should've seen the whole implementation...🤦‍♂️

@ryfu-msft
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ryfu-msft ryfu-msft merged commit 16564d3 into microsoft:main Aug 16, 2023
4 checks passed
@mdanish-kh mdanish-kh deleted the quotedYamlStrings branch August 16, 2023 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When Version is a Floating Point Number, it is Not Quoted
3 participants