GV

GeezerVison

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

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Fix GetLocationCommand output type parameter set and style issues by Meir017 · Pull Request #8324 · PowerShell/PowerShell

Conversation

@Meir017

@Meir017 Meir Blachman (Meir017) commented Nov 20, 2018 •
edited by iSazonov
Loading

Uh oh!

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

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

  • 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:
  • 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

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Meir Blachman (Meir017) added 3 commits July 3, 2018 23:47
@Meir017
update docs for ArgumentCompleterAttribute class
abe1caf
@Meir017
more code comments
3607e6a
@Meir017
fixed GetLocationCommand output type
1a8e8aa
@Meir017
Meir Blachman (Meir017) requested review from Aditya Patwardhan (adityapatwardhan) and Dongbo Wang (daxian-dbw) as code owners November 20, 2018 20:32
Meir017
Meir Blachman (Meir017) commented Nov 20, 2018
View reviewed changes
Comment thread src/Microsoft.PowerShell.Commands.Management/commands/management/Navigation.cs Outdated
[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 })]

@Meir017 Meir Blachman (Meir017) Nov 20, 2018

Copy link
Copy Markdown
Contributor 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.

the output was set to a non existing ParameterSetName

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@Meir017

Meir Blachman (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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
iSazonov
Ilya (iSazonov) reviewed Nov 21, 2018
View reviewed changes

@iSazonov Ilya (iSazonov) left a comment

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.

Please use our template for PR description.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/Microsoft.PowerShell.Commands.Management/commands/management/Navigation.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.
Meir Blachman (Meir017) added 3 commits November 21, 2018 19:06
@Meir017
Merge remote-tracking branch 'origin/master' into patch-1
e0ea79e
@Meir017
Merge branch 'patch-1' of https://github.com/Meir017/PowerShell into …
9b64161 
…patch-1
@Meir017
cr + codefactor
eee3694
@Meir017 Meir Blachman (Meir017) changed the title fixed GetLocationCommand output type fixed GetLocationCommand output type parameter set Nov 21, 2018
iSazonov
Ilya (iSazonov) reviewed Nov 22, 2018
View reviewed changes

@iSazonov Ilya (iSazonov) left a comment

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/Microsoft.PowerShell.Commands.Management/commands/management/Navigation.cs
/// <summary>
/// The string declaration for the Location parameter set in this command.
/// </summary>
private const string PathParameterSet = "Path";

@iSazonov Ilya (iSazonov) Nov 22, 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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/Microsoft.PowerShell.Commands.Management/commands/management/Navigation.cs Outdated
return;
}
}
} // ProcessRecord

@iSazonov Ilya (iSazonov) Nov 22, 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.

Please remove the comment.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Meir Blachman (Meir017) added 2 commits November 22, 2018 22:24
@Meir017
cr
41d5752
@Meir017
Merge remote-tracking branch 'origin/master' into patch-1
fe46112
iSazonov
Ilya (iSazonov) reviewed Nov 23, 2018
View reviewed changes
Comment thread src/Microsoft.PowerShell.Commands.Management/commands/management/Navigation.cs Outdated
_names = value;
} // set
} // LiteralName
} //

@iSazonov Ilya (iSazonov) Nov 23, 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.

Please remove //

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/Microsoft.PowerShell.Commands.Management/commands/management/Navigation.cs Outdated
}
get => _force;
set => _force = value;
} // Force

@iSazonov Ilya (iSazonov) Nov 23, 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.

Please remove '// Force'

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@iSazonov

Ilya (iSazonov) commented Nov 23, 2018

Copy link
Copy Markdown
Collaborator

Meir Blachman (@Meir017) Please look test failures - something was broken in first commits.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Meir Blachman (Meir017) added 3 commits November 23, 2018 08:06
@Meir017
removed deleted code, oops
c742379
@Meir017
formatting
4c3f1ef
@Meir017
oops
cf4bc2b
@Meir017

Meir Blachman (Meir017) commented Nov 23, 2018 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

Ilya (@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
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.
@Meir017

Meir Blachman (Meir017) commented Nov 25, 2018

Copy link
Copy Markdown
Contributor Author

Ilya (@iSazonov) Done
All reactions

Sorry, something went wrong.

Uh oh!

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

Ilya (iSazonov) commented Nov 25, 2018

Copy link
Copy Markdown
Collaborator

Meir Blachman (@Meir017) Thanks for your contribution!

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Meir017
codefactor
3224d29
@Meir017

Meir Blachman (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
All reactions

Sorry, something went wrong.

Uh oh!

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

Rain Sallow (/u/ta11ow) (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. 😄
All reactions

Sorry, something went wrong.

Uh oh!

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

Ilya (iSazonov) commented Nov 26, 2018

Copy link
Copy Markdown
Collaborator

Restart CI-macos.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iSazonov Ilya (iSazonov) self-assigned this Nov 26, 2018
@iSazonov
Ilya (iSazonov) requested review from Steve Lee (SteveL-MSFT) and Travis Plunk (TravisEz13) November 26, 2018 03:59
@iSazonov Ilya (iSazonov) changed the title fixed GetLocationCommand output type parameter set Fix GetLocationCommand output type parameter set and style issues Nov 26, 2018
TravisEz13
Travis Plunk (TravisEz13) approved these changes Nov 26, 2018
View reviewed changes
@iSazonov
Ilya (iSazonov) merged commit 58bbfe8 into PowerShell:master Nov 27, 2018
@iSazonov

Ilya (iSazonov) commented Nov 27, 2018

Copy link
Copy Markdown
Collaborator

Meir Blachman (@Meir017) Thanks for your contribution!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Meir017
Meir Blachman (Meir017) deleted the patch-1 branch November 27, 2018 05:59
@Meir017 Meir Blachman (Meir017) mentioned this pull request Nov 27, 2018
[improvement] removed comments after closing brackets #8344
Merged
7 of 11 tasks
Ilya (iSazonov) pushed a commit to iSazonov/PowerShell that referenced this pull request Nov 29, 2018
@Meir017 @iSazonov
Fix GetLocationCommand output type parameter set and style issues (Po…
7ebc5e1 
…werShell#8324)
@iSazonov Ilya (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 Aditya Patwardhan (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
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026
@Meir017 @iSazonov
Fix GetLocationCommand output type parameter set and style issues (Po…
55059d0 
…werShell#8324)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iSazonov Ilya (iSazonov) iSazonov left review comments
@TravisEz13 Travis Plunk (TravisEz13) TravisEz13 approved these changes
@adityapatwardhan Aditya Patwardhan (adityapatwardhan) Awaiting requested review from adityapatwardhan
@daxian-dbw Dongbo Wang (daxian-dbw) Awaiting requested review from daxian-dbw
@SteveL-MSFT Steve Lee (SteveL-MSFT) Awaiting requested review from SteveL-MSFT

Uh oh!

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

Assignees

@iSazonov Ilya (iSazonov)

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

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

5 participants

@Meir017 @iSazonov @vexx32 @TravisEz13 @adityapatwardhan
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.