-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Format-Hex: Improve mixed-collection piped input and piped streams of input #8674
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
Conversation
RE the Codacy issues - I'll take care of the rest. The Static Analysis link failure is unrelated, possibly an older page was taken down:
EDIT: Seems fine now. Weird. |
@vexx32 Thanks for the great contribution! |
@iSazonov yeah, that makes sense. Will do! |
Close & Reopen to restart failing xUnit test on Windows. Don't think that should have been affected by anything here. |
/cc @iSazonov @JamesWTruher @PaulHigin Style changes PR was #8683 and is merged. This one's just pending your guys review, I think? Let me know. 😄 |
This PR has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed if no further activity occurs within 10 days. |
Ping @PaulHigin for review. 😃 |
...rosoft.PowerShell.Commands.Utility/commands/utility/FormatAndOutput/format-hex/Format-Hex.cs
Show resolved
Hide resolved
@vexx32 I reviewed this again. I think we could close the PR as controversial and continue the discussion in an issue:
I doubt it is necessary. Perhaps this is exactly what users want in some scenarios.
Looks useful in all scenarious. |
If we must, we must. 🤷♂️ I'm inclined to think that users would find its current behaviour when piping a byte stream/array to it extremely useless, but that's just me. But if I'm honest, it may well be just that it doesn't need the behaviour as it isn't used extensively. |
This PR has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed if no further activity occurs within 10 days. |
@vexx32 Please resolve the merge conflict. I think we need rebase to get latest CIs updates. |
5de1539
to
d405003
Compare
@iSazonov done and done. Let's see how CI runs fare. 😄 |
@anmenaga ping for review. 🙂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not blocking.
one comment about the new tests (and i suppose a reflection on all the tests for this).
test/powershell/Modules/Microsoft.PowerShell.Utility/Format-Hex.Tests.ps1
Outdated
Show resolved
Hide resolved
@iSazonov updated tests to account for behavioural changes and updated the PR description to hopefully clarify what's going on. If anything still isn't clear, please let me know! 💖 |
@PoshChan please retry macos |
@vexx32, successfully started retry of |
@PoshChan Please remind me in 1 day |
@iSazonov, this is the reminder you requested 1 day ago |
@SteveL-MSFT @TravisEz13 @rkeithhill The PR is ready for final code review. |
} | ||
else | ||
{ | ||
WriteHexadecimal(bytes, offset: 0, label: GetGroupLabel(typeof(string))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should either consistently name the label param or don't, but it's a bit inconsistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'll opt for naming it, since one of the other possible overloads also has a string param (path
), so it's safer to name this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, one NIT comment
...rosoft.PowerShell.Commands.Utility/commands/utility/FormatAndOutput/format-hex/Format-Hex.cs
Show resolved
Hide resolved
@PoshChan please retry static |
@vexx32, successfully started retry of |
...rosoft.PowerShell.Commands.Utility/commands/utility/FormatAndOutput/format-hex/Format-Hex.cs
Show resolved
Hide resolved
Oh, I guess MSDN is down for a bit. 😄 |
retried the URL check |
Added doc issue reference to PR template: MicrosoftDocs/PowerShell-Docs#4532 |
@vexx32 Thank you! |
🎉 Handy links: |
/cc @iSazonov
PR Summary
Refactoring & Streamed Input Support
Format-Hex -InputObject $byteArray
and$byteArray | Format-Hex
-InputObject
parameter by name.ByteCollection
properties:Label
— stores the type name and a random hexadecimal ID. Used to group objects in output formatting. This can be utilised by, for example,Group-Object
, in order to enable downstream cmdlets to handle each group ofByteCollection
objects separately, if needed.Ascii
— stores the ASCII representation of the bytes in theByteCollection
HexBytes
— stores the hexadecimal representation of the bytes in theByteCollection
Additional tests have been added to cover this behaviour. Details below.
Behaviour of Piped Input
A Primitive Value
System.IO.FileInfo
String
Array of Primitives
Examples
PR Context
Format-Hex needs a bit more versatility and to be able to handle piped input in a more effective manner.
PR Checklist
.h
,.cpp
,.cs
,.ps1
and.psm1
files have the correct copyright headerWIP:
or[ WIP ]
to the beginning of the title (theWIP
bot will keep its status check atPending
while the prefix is present) and remove the prefix when the PR is ready.[feature]
to your commit messages if the change is significant or affects feature tests