-
Notifications
You must be signed in to change notification settings - Fork 8.3k
Add -ExcludeProperty parameter to Format-* cmdlets #26514
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
Changes from all commits
653c9be
c7e04d8
4ca26c5
22364eb
f67ea10
f89d601
20bc7a6
cb19d4a
7438c25
60e17c1
b64a4f9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,7 @@ | |
| using System.Management.Automation; | ||
| using System.Management.Automation.Internal; | ||
| using System.Management.Automation.Runspaces; | ||
| using Microsoft.PowerShell.Commands; | ||
|
|
||
| namespace Microsoft.PowerShell.Commands.Internal.Format | ||
| { | ||
|
|
@@ -729,6 +730,12 @@ public class OuterFormatTableAndListBase : OuterFormatShapeCommandBase | |
| [Parameter(Position = 0)] | ||
| public object[] Property { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// Optional parameter for excluding properties from formatting. | ||
| /// </summary> | ||
| [Parameter] | ||
| public string[] ExcludeProperty { get; set; } | ||
|
|
||
| #endregion | ||
|
|
||
| internal override FormattingCommandLineParameters GetCommandLineParameters() | ||
|
|
@@ -751,6 +758,18 @@ internal override FormattingCommandLineParameters GetCommandLineParameters() | |
|
|
||
| internal void GetCommandLineProperties(FormattingCommandLineParameters parameters, bool isTable) | ||
| { | ||
| // Check View conflicts first (before any auto-expansion) | ||
| if (!string.IsNullOrEmpty(this.View)) | ||
| { | ||
| // View cannot be used with Property or ExcludeProperty | ||
| if ((Property is not null && Property.Length != 0) || (ExcludeProperty is not null && ExcludeProperty.Length != 0)) | ||
| { | ||
| ReportCannotSpecifyViewAndProperty(); | ||
| } | ||
|
|
||
| parameters.viewName = this.View; | ||
| } | ||
|
|
||
| if (Property != null) | ||
| { | ||
| CommandParameterDefinition def; | ||
|
|
@@ -765,15 +784,21 @@ internal void GetCommandLineProperties(FormattingCommandLineParameters parameter | |
| parameters.mshParameterList = processor.ProcessParameters(Property, invocationContext); | ||
| } | ||
|
|
||
| if (!string.IsNullOrEmpty(this.View)) | ||
| if (ExcludeProperty is not null) | ||
| { | ||
| // we have a view command line switch | ||
| if (parameters.mshParameterList.Count != 0) | ||
| parameters.excludePropertyFilter = new PSPropertyExpressionFilter(ExcludeProperty); | ||
|
|
||
| // ExcludeProperty implies -Property * for better UX | ||
| if (Property is null || Property.Length == 0) | ||
| { | ||
| ReportCannotSpecifyViewAndProperty(); | ||
| } | ||
| CommandParameterDefinition def = isTable | ||
| ? new FormatTableParameterDefinition() | ||
| : new FormatListParameterDefinition(); | ||
| ParameterProcessor processor = new ParameterProcessor(def); | ||
| TerminatingErrorContext invocationContext = new TerminatingErrorContext(this); | ||
|
|
||
| parameters.viewName = this.View; | ||
| parameters.mshParameterList = processor.ProcessParameters(new object[] { "*" }, invocationContext); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not for the PR. |
||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -70,6 +70,11 @@ internal sealed class FormattingCommandLineParameters | |||||||||||
| /// Extension mechanism for shape specific parameters. | ||||||||||||
| /// </summary> | ||||||||||||
| internal ShapeSpecificParameters shapeParameters = null; | ||||||||||||
|
|
||||||||||||
| /// <summary> | ||||||||||||
| /// Filter for excluding properties from formatting. | ||||||||||||
| /// </summary> | ||||||||||||
| internal PSPropertyExpressionFilter excludePropertyFilter = null; | ||||||||||||
|
yotsuda marked this conversation as resolved.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Question about the design. We don't keep property filter but only exclude filter. Is this really necessary? Looking how ApplyExcludePropertyFilter is inserted a question is - wouldn't it be more correct to apply the filter exactly at the time of adding new property in the list?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, storing the filter is necessary to pass it from the cmdlet to
Since the cmdlet and Even with add-time filtering, we would still need to add a member to
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My main concern is that we call ApplyExcludePropertyFilter() 8 times. It is looks as not reliable, it is easy to make an error.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the feedback. You're right - calling I've refactored both methods to eliminate multiple
Regarding
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a good step forward. Thanks!
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for the positive feedback and the suggestion! I explored moving Current Structure:
Challenges with base.Initialize():
Possible approach - Template Method pattern: // ViewGenerator (base)
internal virtual void Initialize(...) {
InitializeHelper();
BuildActiveAssociationList(so); // virtual, overridden by derived classes
ApplyExcludePropertyFilter();
}However, this wouldn't work for Wide's lazy initialization pattern without additional refactoring. Current approach: Would you prefer me to explore the Template Method approach further, or is the current single-call-per-Initialize approach acceptable given these constraints?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ideally it would be great to have clean class design. But if it requires a lot of refactoring, then it's better to postpone it, i.e. I can accept the PR as it is now, and you can do the refactoring in a subsequent PR if you want.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for the flexibility and understanding! I agree that a cleaner centralized approach would require some additional refactoring. The current approach—calling I've opened #26568 to track the potential refactoring for the future. Please feel free to assign it to me if you'd like.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Feel free to grab the work. We are in beginning of next version dev cycle so we can do formatting and refactoring that way there's enough time to stabilize.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you! I'll take on #26568. |
||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| /// <summary> | ||||||||||||
|
|
||||||||||||
Uh oh!
There was an error while loading. Please reload this page.