Add new Offset and Count parameters to Format-Hex and refactor the cmdlet #7877

iSazonov · 2018-09-27T15:58:59Z

PR Summary

Add new Offset and Count parameters. Resolve Format-Hex should have Count and Offset parameters #3383.
Modify ByteCollection class to support offsets up to UInt64.MaxValue size.
Hide/obsolete Raw parameter because the behavior is by default. Related Can we hide obsolete or internal-use-only helper parameters from syntax diagrams? #7868.
Optimize conversion to bytes to reduce allocations for large input data.

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

rkeithhill · 2018-09-28T02:07:36Z

...rosoft.PowerShell.Commands.Utility/commands/utility/FormatAndOutput/format-hex/Format-Hex.cs

Redundant from at the end of the sentence.

rkeithhill · 2018-09-28T02:09:01Z

...rosoft.PowerShell.Commands.Utility/commands/utility/FormatAndOutput/format-hex/Format-Hex.cs

rkeithhill · 2018-09-28T02:14:56Z

...rosoft.PowerShell.Commands.Utility/commands/utility/FormatAndOutput/format-hex/Format-Hex.cs

SteveL-MSFT · 2018-10-03T22:08:35Z

@PowerShell/powershell-committee reviewed this and ok with the proposed changes

iSazonov · 2018-10-04T05:16:31Z

Doc Issue created.

TravisEz13 · 2018-10-04T18:16:17Z

test/powershell/Modules/Microsoft.PowerShell.Utility/FormatHex.Tests.ps1

Is this not a breaking change?

@PowerShell/powershell-committee discussed this and formatting changes have not been considered breaking changes. The extra zeros are acceptable.

iSazonov · 2018-10-12T09:17:24Z

@SteveL-MSFT @TravisEz13 Could you please review the PR?

SteveL-MSFT · 2018-10-12T17:50:54Z

...rosoft.PowerShell.Commands.Utility/commands/utility/FormatAndOutput/format-hex/Format-Hex.cs

+                case Int32 iInt32:
+                    inputBytes = BitConverter.GetBytes(iInt32);
+                    break;
+                case Int32[] inputInt32s:


NIT: variable naming is inconsistent with line 257, perhaps just use i32 and i32s? Same below for int64

SteveL-MSFT · 2018-10-12T17:53:28Z

...rosoft.PowerShell.Commands.Utility/commands/utility/FormatAndOutput/format-hex/Format-Hex.cs

+        /// <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)


Since this method does the actual WriteObject(), perhaps rename as WriteHexidecimal()?

SteveL-MSFT · 2018-10-12T17:55:22Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/UtilityCommon.cs

+            StringBuilder asciiEnd = new StringBuilder(BytesPerLine);
+
+            // '+1' comes from 'result.Append(nextLine.ToString() + " " + asciiEnd.ToString());' below.
+            StringBuilder result = new StringBuilder(nextLine.Capacity+asciiEnd.Capacity+1);


NIT: have whitespace between operators: nextLine.Capacity + asciiEnd.Capacity + 1

SteveL-MSFT · 2018-10-12T17:57:48Z

test/powershell/Modules/Microsoft.PowerShell.Utility/FormatHex.Tests.ps1

+            { Format-Hex -Path $inputFile4 -Count 0 } | Should -Throw -ErrorId "ParameterArgumentValidationError,Microsoft.PowerShell.Commands.FormatHex"
+        }
+
+        It "Offset should be >= 0" {


May be interesting to read from end of file, but is out of scope of this PR :)

SteveL-MSFT

LGTM

iSazonov added the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Sep 27, 2018

iSazonov requested review from JamesWTruher and PaulHigin as code owners September 27, 2018 15:59

iSazonov added the Documentation Needed in this repo Documentation is needed in this repo label Sep 27, 2018

iSazonov requested a review from SteveL-MSFT September 27, 2018 15:59

rkeithhill reviewed Sep 28, 2018

View reviewed changes

...rosoft.PowerShell.Commands.Utility/commands/utility/FormatAndOutput/format-hex/Format-Hex.cs Outdated

Copy link

Collaborator

rkeithhill Sep 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

iSazonov changed the title ~~WIP: Add new Offset and Count parameters to Format-Hex and refactor the cmdlet~~ Add new Offset and Count parameters to Format-Hex and refactor the cmdlet Sep 28, 2018

iSazonov mentioned this pull request Sep 28, 2018

Can we hide obsolete or internal-use-only helper parameters from syntax diagrams? #7868

Closed

iSazonov force-pushed the format-hex-offset branch from d435640 to 59f3c37 Compare October 3, 2018 14:20

SteveL-MSFT added Committee-Reviewed PS-Committee has reviewed this and made a decision and removed Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Oct 3, 2018

iSazonov mentioned this pull request Oct 4, 2018

Add new Offset and Count parameters to Format-Hex MicrosoftDocs/PowerShell-Docs#2986

Closed

8 tasks

iSazonov requested review from TravisEz13 and daxian-dbw October 4, 2018 05:17

SteveL-MSFT requested a review from anmenaga October 4, 2018 16:31

TravisEz13 reviewed Oct 4, 2018

View reviewed changes

iSazonov added 4 commits October 12, 2018 14:17

Add Count and Offset parameters in Format-Hex

82138c2

Reduce allocations

5b20bf8

Add tests

8619cd0

Hide deprecated Raw parameter

314b6f5

Fix typo in comment.

578095f

iSazonov force-pushed the format-hex-offset branch from 59f3c37 to 578095f Compare October 12, 2018 09:17

SteveL-MSFT requested changes Oct 12, 2018

View reviewed changes

Address feedback

533f02e

joeyaiello mentioned this pull request Oct 15, 2018

Docs needed for 'Add new Offset and Count parameters to Format-Hex and refactor the cmdlet' MicrosoftDocs/PowerShell-Docs#3020

Closed

joeyaiello removed Documentation Needed in this repo Documentation is needed in this repo labels Oct 15, 2018

SteveL-MSFT approved these changes Oct 18, 2018

View reviewed changes

iSazonov merged commit 745e305 into PowerShell:master Oct 18, 2018

iSazonov deleted the format-hex-offset branch October 18, 2018 05:04

iSazonov mentioned this pull request Oct 19, 2018

Style fixes in Format-Hex #8083

Merged

11 tasks

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 #7877

Uh oh!

Conversation

iSazonov commented Sep 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

Additional comments

PR Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rkeithhill Sep 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteveL-MSFT commented Oct 3, 2018

Uh oh!

iSazonov commented Oct 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov commented Oct 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov Oct 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteveL-MSFT left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

iSazonov commented Sep 27, 2018 •

edited

Loading

rkeithhill Sep 28, 2018 •

edited

Loading

iSazonov Oct 15, 2018 •

edited

Loading