Skip to content

Conversation

MiaRomero
Copy link
Member

@MiaRomero MiaRomero commented Mar 13, 2017

Addressing issue #2730 'Rewrite Format-Hex in C#'

  • Documentation for Format-Hex states both the Path and LiteralPath parameters accept string arrays, though this functionality was not actually present for the cmdlet. This has been corrected so both parameters can accept and process an array of strings. If an invalid path is included in the array, a non-terminating 'FileNotFound' error will be thrown, but the remaining valid paths will be processed and output as usual.

  • After a discussion with @Francisco-Gamino and @joeyaiello, it was decided to make the Raw parameter No-op. Going forward all of the output will be displayed with a true representation of numbers that includes all of the bytes for its type (what the Raw parameter was formally doing). For example, [Int32]170 passed to Format-Hex would have previously displayed only "AA". Now it will display "AA 00 00 00".

  • Updated the InputObject parameter to accept an Int32[], in addition to all previously accepted types.

  • The way Format-Hex processes and displays input from the pipeline has been corrected. Previously, the output for numbers in an array or range would be combined into one as seen below:

  PS D:\GitHub\PowerShell> 1,2 | fhx


           Path:

           00 01 02 03 04 05 06 07 08 09 0A 0B 0C 0D 0E 0F

  00000000   01 02                                            ..

Now each number will be displayed as its own input object:

  PS D:\GitHub\PowerShell> 1,2 |fhx


           00 01 02 03 04 05 06 07 08 09 0A 0B 0C 0D 0E 0F

  00000000   01 00 00 00                                      ....


           00 01 02 03 04 05 06 07 08 09 0A 0B 0C 0D 0E 0F

  00000000   02 00 00 00                                      ....
  • Added a new error that states Format-Hex only supports FileSystem Provider Paths. Previously an error was thrown, but was not specific about the problem (and was displaying the wrong path).
fhx : The given path 'Cert:\CurrentUser\My\' is not supported. This command only supports the FileSystem Provider paths.
At line:1 char:1
+ fhx -path Cert:\CurrentUser\My\
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : InvalidArgument: (Cert:\CurrentUser\My\:String) [Format-Hex], ArgumentException
    + FullyQualifiedErrorId : FormatHexOnlySupportsFileSystemPaths,Microsoft.PowerShell.Commands.FormatHex
  • Cleaned up how we are exposing the Format-Hex error messages. Removed from UtilityCommon.cs and are being accessed through UtilityCommonStrings.resx instead.

  • Updated and added tests for this cmdlet to validate newly implemented functionality and to be more comprehensive for original functionality.

@msftclas
Copy link

@MiaRomero,
Thanks for your contribution.
To ensure that the project team has proper rights to use your work, please complete the Contribution License Agreement at https://cla.microsoft.com.

It will cover your contributions to all Microsoft-managed open source projects.
Thanks,
Microsoft Pull Request Bot

@Francisco-Gamino
Copy link
Contributor

@SandeepSutari, @mirichmo, @daxian-dbw, @joeyaiello: Could you please take a look? Thanks.

@Francisco-Gamino Francisco-Gamino requested review from JamesWTruher and removed request for JamesWTruher March 13, 2017 19:37
@lzybkr lzybkr assigned lzybkr and unassigned MiaRomero Mar 13, 2017
/// </summary>
/// <param name="encoding"></param>
/// <returns></returns>
private Encoding GetEncoding(string encoding)

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

I mean method name...var name is fine

Copy link
Member Author

Choose a reason for hiding this comment

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

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

{
string inputObject = obj.ToString();
byte[] inputBytes = GetEncoding(_encoding).GetBytes(inputObject);
ConvertToHexidecimal(inputBytes, null, 0);

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

/// that array on to the ConvertToHexidecimal method to output.
/// </summary>
/// <param name="_inputObject"></param>
private void ProcessObjectContent(PSObject _inputObject)

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

else
{
string errorMessage = StringUtil.Format(UtilityCommonStrings.FormatHexTypeNotSupported, obj.GetType());
ErrorRecord errorRecord = new ErrorRecord(new ArgumentException(errorMessage),

Choose a reason for hiding this comment

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

I would fix indentation here

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed for all error messages.

{
List<byte> inputStreamArray = new List<byte>();
Int64[] inputInts = (Int64[])obj;
foreach (Int64 i64 in inputInts)

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

/// <returns></returns>
private Encoding GetEncoding(string encoding)
{
if (String.Equals(encoding, "BigEndianUnicode", StringComparison.OrdinalIgnoreCase))

Choose a reason for hiding this comment

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

use string constants instead of raw strings

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

Copy link

@SandeepSutari SandeepSutari left a comment

Choose a reason for hiding this comment

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

here are a few high level comments

githublog.log Outdated
@@ -0,0 +1,1225 @@
**********************
Copy link
Member

Choose a reason for hiding this comment

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

this file shouldn't be part of the PR

[ValidateNotNullOrEmpty()]
public string[] Path
{
get { return _path; }
Copy link
Member

Choose a reason for hiding this comment

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

convention is to use automatic properties:

public string[] Path { get; set; }

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@iSazonov
Copy link
Collaborator

@MiaRomero Do you use the same e-mail for GitHub and CLA sign?

@iSazonov iSazonov added the Breaking-Change breaking change that may affect users label Mar 14, 2017
@iSazonov
Copy link
Collaborator

Add Breaking-Change because -Raw removed.

@msftclas
Copy link

@MiaRomero, thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request.

Thanks, Microsoft Pull Request Bot

/// <summary>
/// Displays the hexidecimal equivalent of the input data.
/// <summary>
[Cmdlet(VerbsCommon.Format, "Hex", SupportsShouldProcess = true, HelpUri ="https://go.microsoft.com/fwlink/?LinkId=821773")]
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be https?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

/// Type of character encoding for InputObject
/// </summary>
[Parameter(ParameterSetName = "ByInputObject")]
[ValidateSet("Ascii", "UTF32", "UTF7", "UTF8", "BigEndianUnicode", "Unicode")]
Copy link
Contributor

Choose a reason for hiding this comment

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

As per @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:

internal static Encoding GetEncodingFromEnum(TextEncodingType type)

Copy link
Collaborator

Choose a reason for hiding this comment

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

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


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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

{
// Write a non-terminating error message indicating that path specified is not supported.
string errorMessage = StringUtil.Format(UtilityCommonStrings.FormatHexOnlySupportsFileSystemPaths, currentPath);
ErrorRecord errorRecord = new ErrorRecord(new ArgumentException(errorMessage),
Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix the indentation of all the errorrecords.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

{
List<byte> inputStreamArray = new List<byte>();
Int64[] inputInts = (Int64[])obj;
foreach (Int64 i64 in inputInts)
Copy link
Contributor

Choose a reason for hiding this comment

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

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


It "Validate the alias for Format-Hex cmdlet 'Get-Command fhx -ErrorAction Stop'" {

try
Copy link
Contributor

Choose a reason for hiding this comment

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

This test case could probably be simplified to:

{ Get-Alias fhx } | Should Not Throw

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

It "ValidateInteger64" {
$invalidPath = "$($inputFile1.DirectoryName)\fakefile8888845345345348709.txt"
$invalid2 = "C:\Windows\fakefile99342039847.txt"
$a = $null
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

InputObjectErrorCase = $true
Path = $inputFile1
InputObject = @{ "hash" = "table" }
expectedError = "FormatHexTypeNotSupported,Microsoft.PowerShell.Commands.FormatHex"
Copy link
Contributor

Choose a reason for hiding this comment

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

'expectedError' should be rename to 'expectedFullyQualifiedErrorId'.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

$actualResult | should be "00000000 41 42 43 44 ABCD "
Context "Correct Expected Errors" {

$otherProvider = Get-ChildItem Cert:\CurrentUser\My\
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

$actualResult = $result.ToString()
# whitespace sensitive
$actualResult | should be "00000000 41 42 43 44 ABCD "
Context "Correct Expected Errors" {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

Name = "Can process int[] type '[int[]](5,6) | fhx'"
inputObject = [int[]](5,6)
Count = 2
Expected = "00000000 05 00 00 00 ...."
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these be ExpectedResult, and ExpectedSecondResult?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

foreach (string currentPath in path)
{
string newPath = Context.SessionState.Path.GetUnresolvedProviderPathFromPSPath(currentPath);
pathsToProcess.Add(newPath);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

{
pathsToProcess.Add(newPaths[0]);
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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)

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@Francisco-Gamino Francisco-Gamino added the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Mar 15, 2017
Copy link
Contributor

@lzybkr lzybkr left a comment

Choose a reason for hiding this comment

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

The file src\Modules\Unix\Microsoft.PowerShell.Utility\Microsoft.PowerShell.Utility.psd1 also needs updating - that should get the tests passing in Travis.

$result | Should Not BeNullOrEmpty
$result.CommandType.ToString() | Should Be "Alias"
,$result | Should BeOfType 'Microsoft.PowerShell.Commands.ByteCollection'
$actualResult = $result.ToString() -split "`r`n"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please use platform independent [Environment]::NewLine

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@MiaRomero
Copy link
Member Author

MiaRomero commented Mar 15, 2017

Hi @SandeepSutari, @Francisco-Gamino, @SteveL-MSFT, @iSazonov, and @JamesWTruher,
Thank you for your feedback. I have addressed your comments - would you please take another look?
Thanks,
Maria

@Francisco-Gamino
Copy link
Contributor

Francisco-Gamino commented Mar 15, 2017

@iSazonov, @joeyaiello: I am removing "Breaking-Change" label as the -Raw parameter is still present (is a No-op).

@Francisco-Gamino Francisco-Gamino removed the Breaking-Change breaking change that may affect users label Mar 15, 2017
Copy link
Member

@SteveL-MSFT SteveL-MSFT left a comment

Choose a reason for hiding this comment

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

My comments have been addressed. Thanks @MiaRomero !

@lzybkr lzybkr added the Breaking-Change breaking change that may affect users label Mar 15, 2017
@lzybkr
Copy link
Contributor

lzybkr commented Mar 15, 2017

@Francisco-Gamino - Even with the noop -Raw parameter - there is a change in behavior we need to document - so please keep the Breaking Change label.

@daxian-dbw
Copy link
Member

It looks like we are using little-endian, since all windows version are little-endian. But since we are going cross-plat, maybe considering supporting big-endian as well? Or maybe at least document that we use little-endian by default?

@lzybkr
Copy link
Contributor

lzybkr commented Mar 15, 2017

@daxian-dbw - I don't think we want to get in the business of dealing with endian-ness in this cmdlet.

I think of it as more of a memory dumping tool, and it shouldn't necessarily interpret what it's dumping (even though we could).

/// <param name="pathsToProcess"></param>
private void ProcessPath(List<string> pathsToProcess)
{
// If there are no paths to proccess, just return.
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

@MiaRomero MiaRomero Mar 20, 2017

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

// will just contain multiple references to the same buffer memory space (containing only the
// last bytes of the file read). Cloning the buffer allows us to pass the values on without
// overwriting previous values.
byte[] bufferClone = (byte[])buffer.Clone();
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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 will follow up. Due to time constraints with my internship, I won't be able to follow up after today.

{
Object obj = inputObject.BaseObject;
byte[] inputBytes = null;
if (obj is System.IO.FileSystemInfo)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

inputBytes = resolvedEncoding.GetBytes(inputString);
}

else if (obj is byte)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@Francisco-Gamino Francisco-Gamino Mar 17, 2017

Choose a reason for hiding this comment

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

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


foreach (string path in pathsToProcess)
{
ProcessFileContent(path);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@MiaRomero MiaRomero Mar 20, 2017

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Great.

@SteveL-MSFT 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 Mar 15, 2017
@SteveL-MSFT
Copy link
Member

@PowerShell/powershell-committee reviewed this and agreed on:

  • for numbers, we should prefix with 0 to retain width
  • change to formatting (due to storing in property) is a breaking change that just needs to be documented
  • if array is passed in, then each element in the array is a separate output object
  • FileInfo retains current behavior (special cased to format the content)

,$result | Should BeOfType 'Microsoft.PowerShell.Commands.ByteCollection'
$actualResult = $result.ToString()
($actualResult -match $inputText1) | Should Be $true
$errorThrown | Should Not Be $null
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems like this is unnecessary and should be removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

}

$thrownError | Should Not Be $null
$thrownError.FullyQualifiedErrorId | Should Match $testCase.ExpectedFullyQualifiedErrorId
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please use the template to check FullyQualifiedErrorId

{ Start-Process -FilePath $pingCommand -WindowStyle Normal } | ShouldBeErrorId "NotSupportedException,Microsoft.PowerShell.Commands.StartProcessCommand"

Copy link
Member Author

Choose a reason for hiding this comment

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

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


foreach ($testCase in $testCases)
{
RunPathAndLiteralPathParameterTestCase $testCase
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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


It "Validate the alias (fhx) for Format-Hex cmdlet is available" {

{ Get-Alias fhx } | Should Not Throw
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed.

{
private const int BUFFERSIZE = 16;

#region Parameters
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@rkeithhill, please open a issue for that suggestion. Thanks.

@MiaRomero MiaRomero force-pushed the formatHex branch 2 times, most recently from 971954d to 41f6b10 Compare March 21, 2017 04:37
@MiaRomero
Copy link
Member Author

Thank you everyone for your feedback. @lzybkr can you please merge when you have a chance? My internship is over so if there are any questions/concerns please address them to @Francisco-Gamino. Thanks!

@Francisco-Gamino
Copy link
Contributor

It was great working with you @MiaRomero, thank you!

Copy link
Contributor

@lzybkr lzybkr left a comment

Choose a reason for hiding this comment

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

I opened #3382 so we clean up the test a little, but otherwise looks good.

@lzybkr lzybkr merged commit 0e12fbf into PowerShell:master Mar 21, 2017
@Francisco-Gamino
Copy link
Contributor

Thanks @lzybkr! I will take care of #3382.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking-Change breaking change that may affect users Committee-Reviewed PS-Committee has reviewed this and made a decision
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants