Add new Offset and Count parameters to Format-Hex and refactor the cmdlet#7877
Add new Offset and Count parameters to Format-Hex and refactor the cmdlet#7877iSazonov merged 6 commits intoPowerShell:masterfrom
Conversation
There was a problem hiding this comment.
Redundant from at the end of the sentence.
d435640 to
59f3c37
Compare
|
@PowerShell/powershell-committee reviewed this and ok with the proposed changes |
|
Doc Issue created. |
There was a problem hiding this comment.
Is this not a breaking change?
There was a problem hiding this comment.
@PowerShell/powershell-committee discussed this and formatting changes have not been considered breaking changes. The extra zeros are acceptable.
59f3c37 to
578095f
Compare
|
@SteveL-MSFT @TravisEz13 Could you please review the PR? |
| case Int32 iInt32: | ||
| inputBytes = BitConverter.GetBytes(iInt32); | ||
| break; | ||
| case Int32[] inputInt32s: |
There was a problem hiding this comment.
NIT: variable naming is inconsistent with line 257, perhaps just use i32 and i32s? Same below for int64
| /// <param name="inputBytes">Bytes for the hexadecimial representaion.</param> | ||
| /// <param name="path">File path.</param> | ||
| /// <param name="offset">Offset in the file.</param> | ||
| private void ConvertToHexidecimal(Span<byte> inputBytes, string path, Int64 offset) |
There was a problem hiding this comment.
Since this method does the actual WriteObject(), perhaps rename as WriteHexidecimal()?
| StringBuilder asciiEnd = new StringBuilder(BytesPerLine); | ||
|
|
||
| // '+1' comes from 'result.Append(nextLine.ToString() + " " + asciiEnd.ToString());' below. | ||
| StringBuilder result = new StringBuilder(nextLine.Capacity+asciiEnd.Capacity+1); |
There was a problem hiding this comment.
NIT: have whitespace between operators: nextLine.Capacity + asciiEnd.Capacity + 1
| { Format-Hex -Path $inputFile4 -Count 0 } | Should -Throw -ErrorId "ParameterArgumentValidationError,Microsoft.PowerShell.Commands.FormatHex" | ||
| } | ||
|
|
||
| It "Offset should be >= 0" { |
There was a problem hiding this comment.
May be interesting to read from end of file, but is out of scope of this PR :)
PR Summary
Additional comments
For review: Now Offset parameter does a shift in output byte stream. Since we accept objects as input we could consider Offset as "skip the Offset-count objects from input (array/pipeline)" although Skip parameter is more suitable for this (or Select-Object -Skip) and this is contrary to the behavior for input streams (Path parameter or FileInfo input object).
I don't consider a scenario "get last count bytes". As a minimum, the PR should not block this enhancement.
New ReFS file system support files up to UInt64.MaxValue. So ByteCollection is modified to support UInt64.MaxValue too.
On the other hand current .Net Core limitation is Int64.MaxValue for file size/offset. In result we have to use Int64 type for Offset and Count parameters.
/cc @rkeithhill @mklement0 @adamdriscoll
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:to the beginning of the title and remove the prefix when the PR is ready.[feature]if the change is significant or affects feature tests