Skip to content

Fix GetLocationCommand output type parameter set and style issues #8324

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 12 commits into from
Nov 27, 2018
Merged

Fix GetLocationCommand output type parameter set and style issues #8324

merged 12 commits into from
Nov 27, 2018

Conversation

Meir017
Copy link
Contributor

@Meir017 Meir017 commented Nov 20, 2018

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
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
Copy link
Contributor Author

Meir017 commented Nov 20, 2018

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

Copy link
Collaborator

@iSazonov iSazonov left a comment

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.

@Meir017 Meir017 changed the title fixed GetLocationCommand output type fixed GetLocationCommand output type parameter set Nov 21, 2018
Copy link
Collaborator

@iSazonov iSazonov left a comment

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

@@ -3739,7 +3755,7 @@ private void RenameItem(string path, bool literalPath = false)
pathNotFound));
return;
}
}
} // ProcessRecord
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 comment.

set
{
base.SuppressWildcardExpansion = true;
_names = value;
} // set
} // LiteralName
} //
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 //

_force = value;
}
get => _force;
set => _force = value;
} // Force
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 '// Force'

@iSazonov
Copy link
Collaborator

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

@Meir017
Copy link
Contributor Author

Meir017 commented Nov 23, 2018

@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
Copy link
Contributor Author

Meir017 commented Nov 25, 2018

@iSazonov Done

@iSazonov
Copy link
Collaborator

@Meir017 Thanks for your contribution!

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

@Meir017
Copy link
Contributor Author

Meir017 commented Nov 25, 2018

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

@vexx32
Copy link
Collaborator

vexx32 commented Nov 25, 2018

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
Collaborator

Restart CI-macos.

@iSazonov iSazonov self-assigned this Nov 26, 2018
@iSazonov iSazonov changed the title fixed GetLocationCommand output type parameter set Fix GetLocationCommand output type parameter set and style issues Nov 26, 2018
@iSazonov iSazonov merged commit 58bbfe8 into PowerShell:master Nov 27, 2018
@iSazonov
Copy link
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants