GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Add new Offset and Count parameters to Format-Hex and refactor the cmdlet by iSazonov · Pull Request #7877 · PowerShell/PowerShell

Conversation

Ilya (iSazonov) commented Sep 27, 2018 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Collaborator

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 Keith Hill (@rkeithhill) Michael Klement (@mklement0) Adam Driscoll (@adamdriscoll)

PR Checklist

PR has a meaningful title
- Use the present tense and imperative mood when describing your changes
Summarized changes
Change is not breaking
Make sure all .h, .cpp, .cs, .ps1 and .psm1 files have the correct copyright header
This PR is ready to merge and is not Work in Progress.
- If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready.
User-facing changes
- Not Applicable
- OR
- User-facing Documentation needed
  - Issue filed - Issue link: Add new Offset and Count parameters to Format-Hex MicrosoftDocs/PowerShell-Docs#2986
Testing - New and feature
- Not Applicable or can only be tested interactively
- OR
- Make sure you've added a new test if existing tests do not effectively test the code changed
  - Add [feature] if the change is significant or affects feature tests

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

mklement0 reacted with thumbs up emoji

rkeithhill reacted with hooray emoji

rkeithhill reacted with heart emoji

All reactions

👍 1 reaction
🎉 1 reaction
❤️ 1 reaction

Ilya (iSazonov) added the Review - Committee The PR/Issue needs a review from the PowerShell Committee label

Sep 27, 2018

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 Documentation is needed in this repo label

Sep 27, 2018

Ilya (iSazonov) requested a review from Steve Lee (SteveL-MSFT)

September 27, 2018 15:59

Keith Hill (rkeithhill) reviewed

Sep 28, 2018

View reviewed changes

Comment thread

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

Keith Hill (rkeithhill) Sep 28, 2018

Copy link

Copy Markdown

Collaborator

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.

Redundant from at the end of the sentence.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Ilya (iSazonov) Sep 28, 2018

Copy link

Copy Markdown

Collaborator Author

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.

Fixed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Keith Hill (rkeithhill) reviewed

Sep 28, 2018

View reviewed changes

Comment thread

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

Keith Hill (rkeithhill) Sep 28, 2018 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Collaborator

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.

👍

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Keith Hill (rkeithhill) reviewed

Sep 28, 2018

View reviewed changes

Comment thread

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

Keith Hill (rkeithhill) Sep 28, 2018

Copy link

Copy Markdown

Collaborator

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.

Nice!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Ilya (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

Ilya (iSazonov) mentioned this pull request

Sep 28, 2018

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

Closed

Ilya (iSazonov) force-pushed the format-hex-offset branch from d435640 to 59f3c37 Compare

October 3, 2018 14:20

Steve Lee (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

Steve Lee (SteveL-MSFT) commented Oct 3, 2018

Copy link

Copy Markdown

Member

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

iSazonov reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Ilya (iSazonov) mentioned this pull request

Oct 4, 2018

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

Closed

2 of 8 tasks

Ilya (iSazonov) commented Oct 4, 2018

Copy link

Copy Markdown

Collaborator Author

Doc Issue created.

SteveL-MSFT reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Ilya (iSazonov) requested review from Travis Plunk (TravisEz13) and Dongbo Wang (daxian-dbw)

October 4, 2018 05:17

Steve Lee (SteveL-MSFT) requested a review from Andrew (anmenaga)

October 4, 2018 16:31

Travis Plunk (TravisEz13) reviewed

Oct 4, 2018

View reviewed changes

Comment thread

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

Travis Plunk (TravisEz13) Oct 4, 2018

Copy link

Copy Markdown

Member

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.

Is this not a breaking change?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Steve Lee (SteveL-MSFT) Oct 4, 2018

Copy link

Copy Markdown

Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

TravisEz13 and iSazonov reacted with thumbs up emoji

All reactions

👍 2 reactions

Ilya (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

Ilya (iSazonov) force-pushed the format-hex-offset branch from 59f3c37 to 578095f Compare

October 12, 2018 09:17

Ilya (iSazonov) commented Oct 12, 2018

Copy link

Copy Markdown

Collaborator Author

Steve Lee (@SteveL-MSFT) Travis Plunk (@TravisEz13) Could you please review the PR?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Steve Lee (SteveL-MSFT) requested changes

Oct 12, 2018

View reviewed changes

Comment thread

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

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

Steve Lee (SteveL-MSFT) Oct 12, 2018

Copy link

Copy Markdown

Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Ilya (iSazonov) Oct 15, 2018

Copy link

Copy Markdown

Collaborator Author

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.

Fixed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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

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

Steve Lee (SteveL-MSFT) Oct 12, 2018

Copy link

Copy Markdown

Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

TravisEz13 reacted with thumbs up emoji

All reactions

👍 1 reaction

Ilya (iSazonov) Oct 15, 2018

Copy link

Copy Markdown

Collaborator Author

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.

Fixed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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

		StringBuilder asciiEnd = new StringBuilder(BytesPerLine);

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

Steve Lee (SteveL-MSFT) Oct 12, 2018

Copy link

Copy Markdown

Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Ilya (iSazonov) Oct 15, 2018 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Collaborator Author

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.

Fixed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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" {

Steve Lee (SteveL-MSFT) Oct 12, 2018

Copy link

Copy Markdown

Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          Address feedback

533f02e

Joey Aiello (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

Joey Aiello (joeyaiello) removed Documentation Needed in this repo Documentation is needed in this repo labels

Oct 15, 2018

Steve Lee (SteveL-MSFT) approved these changes

Oct 18, 2018

View reviewed changes

Steve Lee (SteveL-MSFT) left a comment

Copy link

Copy Markdown

Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Ilya (iSazonov) merged commit 745e305 into PowerShell:master

Oct 18, 2018

Ilya (iSazonov) deleted the format-hex-offset branch

October 18, 2018 05:04

Ilya (iSazonov) mentioned this pull request

Oct 19, 2018

Style fixes in Format-Hex #8083

Merged

6 of 11 tasks

Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request

Aug 6, 2026


          Add new Offset and Count parameters to Format-Hex and refactor the cm…

1a0af46

…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.

Reviewers

Travis Plunk (TravisEz13)

TravisEz13 left review comments

Steve Lee (SteveL-MSFT)

SteveL-MSFT approved these changes

James Truher (JamesWTruher)

Awaiting requested review from JamesWTruher

Paul Higinbotham (PaulHigin)

Awaiting requested review from PaulHigin

Dongbo Wang (daxian-dbw)

Awaiting requested review from daxian-dbw

Andrew (anmenaga)

Awaiting requested review from anmenaga

+1 more reviewer

Keith Hill (rkeithhill)

rkeithhill left review comments

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

Committee-Reviewed

PS-Committee has reviewed this and made a decision

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Format-Hex should have Count and Offset parameters

Uh oh!

There was an error while loading. Please reload this page.

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.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

Ilya (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!

Keith Hill (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!

Steve Lee (SteveL-MSFT) commented Oct 3, 2018

Uh oh!

Ilya (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!

Ilya (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!

Ilya (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!

Steve Lee (SteveL-MSFT) left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Ilya (iSazonov) commented Sep 27, 2018 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Keith Hill (rkeithhill) Sep 28, 2018 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Ilya (iSazonov) Oct 15, 2018 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.