Add new Offset and Count parameters to Format-Hex and refactor the cmdlet - #7877
Merged
Merged
Conversation
Ilya (iSazonov)
added
the
Review - Committee
Ilya (iSazonov)
requested review from
James Truher (JamesWTruher) and
Paul Higinbotham (PaulHigin)
as code owners
September 27, 2018 15:59
Ilya (iSazonov)
added
the
Documentation Needed in this repo
Collaborator
There was a problem hiding this comment.
Redundant from at the end of the sentence.
Ilya (iSazonov)
changed the title
Ilya (iSazonov)
force-pushed
the
format-hex-offset
branch
from
October 3, 2018 14:20
d435640 to
59f3c37
Compare
Steve Lee (SteveL-MSFT)
added
Committee-Reviewed
Member
|
@PowerShell/powershell-committee reviewed this and ok with the proposed changes |
8 tasks
Collaborator
Author
|
Doc Issue created. |
Ilya (iSazonov)
requested review from
Travis Plunk (TravisEz13) and
Dongbo Wang (daxian-dbw)
October 4, 2018 05:17
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.
Ilya (iSazonov)
force-pushed
the
format-hex-offset
branch
from
October 12, 2018 09:17
59f3c37 to
578095f
Compare
Collaborator
Author
|
Steve Lee (@SteveL-MSFT) Travis Plunk (@TravisEz13) Could you please review the PR? |
Steve Lee (SteveL-MSFT)
requested changes
Oct 12, 2018
| 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 :)
Joey Aiello (joeyaiello)
removed
Documentation Needed in this repo
11 tasks
Thatgfsj (Thatgfsj)
pushed a commit
to Thatgfsj/PowerShell
that referenced
this pull request
Aug 6, 2026
…dlet (PowerShell#7877) - Add new Offset and Count parameters. - Modify ByteCollection class to support offsets up to UInt64.MaxValue size. - Hide/obsolete Raw parameter because the behavior is by default. - Optimize conversion to bytes to reduce allocations for large input data.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 Keith Hill (@rkeithhill) Michael Klement (@mklement0) Adam Driscoll (@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