Replace `BinaryFormatter` with `PSSerializer`

microsoft / PowerShellForGitHub

Microsoft PowerShell wrapper for GitHub API

Other

582 stars 184 forks source link

Replace `BinaryFormatter` with `PSSerializer` #415

Closed andyleejordan closed 7 months ago

andyleejordan commented 10 months ago

Description

Since the former has been removed from PowerShell 7.4 (because it was removed from .NET 7) and the latter can accomplish the same thing.

Issues Fixed

Fixes #413

References

N/A

Checklist

[x] You actually ran the code that you just wrote, especially if you did just "one last quick change".
[x] Comment-based help added/updated, including examples.
[x] Static analysis is reporting back clean.
[x] New/changed code adheres to our coding guidelines.
[x] Formatters were created for any new types being added.
[x] New/changed code continues to support the pipeline.
[x] Changes to the manifest file follow the manifest guidance.
[x] Unit tests were added/updated and are all passing. See testing guidelines. This includes making sure that all pipeline input variations have been covered.
[x] Relevant usage examples have been added/updated in USAGE.md.
[x] If desired, ensure your name is added to our Contributors list

andyleejordan commented 10 months ago

Ah we need to figure out what depth is sufficient:

The input object to serialize. Serializes to a default depth of 1.

...I set it to 64.

andyleejordan commented 10 months ago

@HowardWolosky the third commit is a little risker and you may want further testing of it. I verified my previous workflow still worked (and was previously erroring because of the use of BinaryFormatter). @seeminglyscience pointed out to me that the whole function DeepCopy-Object was unnecessary and could be replaced with a simple addition in the loop to add the converted properties to the new clone object. It's an option, and I've left it in a separate commit if you want it (or you can just take the first two).

andyleejordan commented 10 months ago

Thanks so much for bringing this to my attention with the issue, as well as for providing the fix!

Your change looks good (aside from the minor requested capitalization change) from a code perspective. I just need to run it through some tests to verify that there aren't any unexpected regressions that I'm missing from a cursory review.

You're quite welcome! And yes, please verify it further. I use your module in a script to generate a changelog and programmatically create a GitHub release with assets uploaded direct from the CI/CD machines. It's great! I definitely want it to keep working with PowerShell 7.4.

andyleejordan commented 9 months ago

Have you had a chance to test it more thoroughly yet? Thanks!

andyleejordan commented 8 months ago

Hey @HowardWolosky just following up on this as PowerShell 7.4 gets closer to GA (and will result in this module being quite broken without this change). 😄

brogers5 commented 7 months ago

PowerShell 7.4 has since hit GA, and this issue is starting to impact module consumers like myself. Is there anything we can do to help move this PR forward?

andyleejordan commented 7 months ago

Heck yeah, thanks @HowardWolosky!

HowardWolosky commented 7 months ago

This has been published to PowerShellGallery as part of 0.17.0: https://www.powershellgallery.com/packages/PowerShellForGitHub/0.17.0