Skip to content

Fix GetLocationCommand output type parameter set and style issues - #8324

Merged
iSazonov merged 12 commits into
PowerShell:masterfrom
Meir017:patch-1
Nov 27, 2018
Merged

Fix GetLocationCommand output type parameter set and style issues#8324
iSazonov merged 12 commits into
PowerShell:masterfrom
Meir017:patch-1

Conversation

@Meir017

@Meir017 Meir017 commented Nov 20, 2018

Copy link
Copy Markdown
Contributor

PR Summary

Fixes the GetLocationCommand output type parameter-set from the non-existing locationSet to use the constant field LocationParameterSet.
Fix style issues in Navigation.cs.

PR Checklist

perhaps all of the parameter-set-names constants should be in a single place since many of then are used in multiple commands
maybe move them into the base class?

[OutputType(typeof(PathInfo), ParameterSetName = new string[] { "locationSet" })]
[OutputType(typeof(PathInfoStack), ParameterSetName = new string[] { "Stack" })]
[Cmdlet(VerbsCommon.Get, "Location", DefaultParameterSetName = locationSet, SupportsTransactions = true, HelpUri = "https://go.microsoft.com/fwlink/?LinkID=113321")]
[OutputType(typeof(PathInfo), ParameterSetName = new string[] { locationSet })]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

the output was set to a non existing ParameterSetName

@Meir017

Meir017 commented Nov 20, 2018

Copy link
Copy Markdown
Contributor Author

CodeFactor fails because of Constants must start with an upper-case letter but all over the project contants start with a lower-case letter so I wasn't sure about this

@iSazonov iSazonov left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please use our template for PR description.

Comment thread src/Microsoft.PowerShell.Commands.Management/commands/management/Navigation.cs Outdated
@Meir017 Meir017 changed the title fixed GetLocationCommand output type Nov 21, 2018

@iSazonov iSazonov left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@Meir017 Please add more information in PR description about that you want fix. It is not clear wgy you remove GetResolvedPaths and RenameItem.

/// <summary>
/// The string declaration for the Location parameter set in this command.
/// </summary>
private const string PathParameterSet = "Path";

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please remove such obvious comments. I see they already was there but it is bad pattern.
Below too.

return;
}
}
} // ProcessRecord

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please remove the comment.

_names = value;
} // set
} // LiteralName
} //

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please remove //

}
get => _force;
set => _force = value;
} // Force

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please remove '// Force'

@iSazonov

Copy link
Copy Markdown
Collaborator

@Meir017 Please look test failures - something was broken in first commits.

@Meir017

Meir017 commented Nov 23, 2018

Copy link
Copy Markdown
Contributor Author

@iSazonov btw to remove the // get/set/Force... comments we could just use a regex replace:
replace: } // .*
to: }
but I'm guessing it's out of scope for this issue

@Meir017

Meir017 commented Nov 25, 2018

Copy link
Copy Markdown
Contributor Author

@iSazonov Done

@iSazonov

Copy link
Copy Markdown
Collaborator

@Meir017 Thanks for your contribution!

Could you please look CodeFactor issues https://www.codefactor.io/repository/github/powershell/powershell/pull/8324 ?

@Meir017

Meir017 commented Nov 25, 2018

Copy link
Copy Markdown
Contributor Author

many of the changes are about the file src\System.Management.Automation\engine\Attributes.cs not sure about them

@vexx32

vexx32 commented Nov 25, 2018

Copy link
Copy Markdown
Collaborator

Don't worry too much about anything that's in code files you haven't touched; just focus on the handful that are in or around the code you've modified. 😄

@iSazonov

Copy link
Copy Markdown
Collaborator

Restart CI-macos.

@iSazonov iSazonov self-assigned this Nov 26, 2018
@iSazonov iSazonov changed the title fixed GetLocationCommand output type parameter set Nov 26, 2018
@iSazonov
iSazonov merged commit 58bbfe8 into PowerShell:master Nov 27, 2018
@iSazonov

Copy link
Copy Markdown
Collaborator

@Meir017 Thanks for your contribution!

@Meir017
Meir017 deleted the patch-1 branch November 27, 2018 05:59
iSazonov pushed a commit to iSazonov/PowerShell that referenced this pull request Nov 29, 2018
@iSazonov iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Nov 29, 2018
@adityapatwardhan adityapatwardhan added CL-CodeCleanup Indicates that a PR should be marked as a Code Cleanup change in the Change Log and removed CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log labels Dec 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CL-CodeCleanup Indicates that a PR should be marked as a Code Cleanup change in the Change Log

5 participants