Why is the private member not camel cased ? Unless Pascal casing is the recommended pattern for C# cmdlets, we should use camel casing

I mean method name...var name is fine

I talked to Dongbo Wang (@daxian-dbw) about this, and Pascal casing is the recommended pattern for the cmdlets.

Looks like you could get the inputBytes out of each of these else if {} blocks and do the convert* at the end if else block. that would prevent 10 uses of convert calls peppered across this method.

Fixed.

unless this is the pattern or a standard practice to use _ for private members we should avoid it. Also why is the method variable using the private member style...we should stick with camel casing since its a method variable.

parameter names shouldn't have '_' prefixed to them.

Fixed.

I would fix indentation here

Fixed for all error messages.

i64 should get itself a better var name. currently its a type name.

You can do something like foreach (Int64 value in inputInts){...}

Fixed.

use string constants instead of raw strings

Fixed.

this file shouldn't be part of the PR

convention is to use automatic properties:

public string[] Path { get; set; }

Fixed.

The HelpUr should be "http://go.microsoft.com/fwlink/?LinkId=526919". This will fix your failures in the CI test pass.

Shouldn't this be https?

Fixed.

As per Dongbo Wang (@daxian-dbw) suggestion, please see if you can reuse some of existing cmdlets' code that support encoding parameters.

You can probably reuse some of this code:

yes, this should probably be an enum rather than the string set

In reply to: 105936094 [](ancestors = 105936094)

We have FileSystemCmdletProviderEncoding enum (but in EncodingConversion the same is string constants in the class.
It seems we need to bring this to the common code base.)

Fixed.

Please fix the indentation of all the errorrecords.

Fixed.

You can do something like foreach (Int64 value in inputInts){...}

This test case could probably be simplified to:

{ Get-Alias fhx } | Should Not Throw

Fixed.

Could you please use a more meaningful variable name? E.g., $output, and $errorThrown?

Fixed.

'expectedError' should be rename to 'expectedFullyQualifiedErrorId'.

Fixed.

$otherProvider should probably be renamed to '$certificateProvider'.

Fixed.

Could you please rename this Context to something like "Validate Format-Hex error scenarios" or "Validate negative test cases" or "Validate error scenarios"?

Fixed.

Should these be ExpectedResult, and ExpectedSecondResult?

Fixed.

do you need to validate that these paths are filesystem provider paths?
for example, format-hex -literalpath hklm:/software should perhaps produce the same error as format-hex -path hklm:/software

Fixed.

it seems like it would be a useful scenario to support multiple files here:
format-hex file*.txt
unless that's an explicit non-supported scenario

yep - looking below, it appears that it just loops through the provided paths, it seems reasonable to support more than one resolved path

In reply to: 105987086 [](ancestors = 105987086)

Fixed.

Please use platform independent [Environment]::NewLine

Fixed.

The test Count == 0 is not necessary if we aren't reporting any errors, the foreach statement handles that case cleanly and efficiently.

That said, should we report an error if there are no files?

Thank you. Good point, it turns out we do not need this code. It does throw an error if the path does not resolve to a file.

The sample below shows an array of two paths that don't exist. Each will throw a non-terminating error.

fhx -Path "c:\dlfkjasdkfj.txt", "c:\ldkfj.txt"
fhx : Cannot find path 'C:\dlfkjasdkfj.txt' because it does not exist.
At line:1 char:1
+ fhx -Path "c:\dlfkjasdkfj.txt", "c:\ldkfj.txt"
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : ObjectNotFound: (System.String[]:String[]) [Format-Hex], ItemNotFoundException
    + FullyQualifiedErrorId : FileNotFound,Microsoft.PowerShell.Commands.FormatHex

fhx : Cannot find path 'C:\ldkfj.txt' because it does not exist.
At line:1 char:1
+ fhx -Path "c:\dlfkjasdkfj.txt", "c:\ldkfj.txt"
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : ObjectNotFound: (System.String[]:String[]) [Format-Hex], ItemNotFoundException
    + FullyQualifiedErrorId : FileNotFound,Microsoft.PowerShell.Commands.FormatHex

And just to confirm - there is no error if you use a wildcard and there are no matches?

Clone is copying data needlessly here.

Instead, you could pass buffer to ConvertToHexidecimal, then create a new array that is zero initialized. The zeroing of memory by .Net is going to be more efficient than the Clone (which will have started with zeroed memory anyway).

I've changed it to copy the buffer into a new array instead of cloning the buffer. If this isn't what you had in mind, please file an issue and Francisco Gamino (@Francisco-Gamino) will follow up. Due to time constraints with my internship, I won't be able to follow up after today.

I know this is just a translation of the C#, but I don't think this case makes sense.

If I pass an object for Format-Hex, I expect that object to be formatted. I didn't ask for the contents of the file, that's a different parameter set.

Per committee review/comment below, we'll leave as is for now.

It would be nice to see all primitive types (including enums) and arrays of these types - instead of this random assortment.

I wrote some code that covers much of what I think is useful and it's a similar number of lines of code, see https://gist.github.com/lzybkr/9940b63c8301c31316bdb3ec6305536f

It might also be useful to support any value type, not just primitives, but that is a little harder.

Hi Jason Shirk (@lzybkr), Maria Romero (@MiaRomero) Leap Program assignment/internship ends today, so I will follow-up on this. I have open an issue #3358 to track this. Thanks.

It doesn't look like there is any file header, so if there are multiple files, it will be difficult to understand the output.

There is a header for each ByteCollection object, e.g.,

New-Item -Value "foo" -Path foo1.txt -ItemType File
New-Item -Value "foo2" -Path foo2.txt -ItemType File

PS C:\exp> dir foo* | fhx
           Path: C:\exp\foo1.txt
           00 01 02 03 04 05 06 07 08 09 0A 0B 0C 0D 0E 0F
00000000   66 6F 6F                                         foo

           Path: C:\exp\foo2.txt
           00 01 02 03 04 05 06 07 08 09 0A 0B 0C 0D 0E 0F
00000000   66 6F 6F 32                                      foo2

PS C:\exp> fhx -Path .\foo*
           Path: C:\exp\foo1.txt
           00 01 02 03 04 05 06 07 08 09 0A 0B 0C 0D 0E 0F
00000000   66 6F 6F                                         foo

           Path: C:\exp\foo2.txt
           00 01 02 03 04 05 06 07 08 09 0A 0B 0C 0D 0E 0F
00000000   66 6F 6F 32                                      foo2

Great.

It seems like this is unnecessary and should be removed.

In this scenario we want to test that we get both an error and output.

Please use the template to check FullyQualifiedErrorId

I've tried using this but get this error:
The term 'ShouldBeErrorId' is not recognized as the name of a cmdlet, function, script file, or operable program. Check the spelling of the name, or if a path was included, verify that the path is correct and try again.

So for now I'll keep Should Match

We could call the function with parameter PathCase = $true then PathCase = $false

Yes, that is true. But I'll leave as is for now.

Please remove the test because we already have separate files with tests for aliases.

Removed.

Is there any chance we could get Count and Offset parameters? I could probably live without Offset but on the PSCX Format-Hex command, I use -Count almost all the time. That's because I use it to see if a text file has a BOM, in which case I only want to see the first 3 characters.

Those are good suggestions, but out of scope for this PR.

Keith Hill (@rkeithhill), please open a issue for that suggestion. Thanks.