Join GitHub today
GitHub is home to over 36 million developers working together to host and review code, manage projects, and build software together.
Sign upJoin-String cmdlet for creating text from pipeline input #7660
Conversation
powercode
added some commits
Aug 29, 2018
powercode
requested review from
BrucePay,
dantraMSFT,
daxian-dbw,
JamesWTruher and
PaulHigin
as
code owners
Aug 29, 2018
This comment has been minimized.
This comment has been minimized.
|
'Object' in the cmdlet name implies that we can join objects of any type - array + array, array + hash, hash + hash, strings and so on. This cmdlet is more like ConvertTo-String |
This comment has been minimized.
This comment has been minimized.
|
It is intended to be the pipeline equivalent of the |
This comment has been minimized.
This comment has been minimized.
RichardSiddaway
commented
Aug 30, 2018
|
If its going to be the pipeline equivalent of -join wouldn't Join-String be a more descriptive name. Shouldn't there also be a Split-? cmdlet to match the -split operator |
This comment has been minimized.
This comment has been minimized.
|
It depends. It can join objects, or the properties of objects. The output is a string, but it works with object input. Just like Group-Object and Sort-Object. |
PaulHigin
requested changes
Aug 30, 2018
| /// </summary> | ||
| [Cmdlet(VerbsCommon.Join, "Object", RemotingCapability = RemotingCapability.None, DefaultParameterSetName = "default")] | ||
| [OutputType(typeof(string))] | ||
| public class JoinObjectCommand : PSCmdlet |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| public class JoinObjectCommand : PSCmdlet | ||
| { | ||
| // ReSharper disable once CollectionNeverQueried.Local | ||
| private readonly List<PSObject> _inputObjects = new List<PSObject>(50); |
This comment has been minimized.
This comment has been minimized.
PaulHigin
Aug 30, 2018
Contributor
I normally don't see List size initialization unless the size is known. I am curious why you initialize the size to this value?
This comment has been minimized.
This comment has been minimized.
powercode
Aug 31, 2018
Author
Collaborator
It is cheaper to allocate a list that is a little larger than to reallocate and copy when out-grown.
But the size can always be argued.
When I profile memory allocations, Array allocations on List.Resize is not uncommon, and when I see them, I try to find a sensible default size do get rid of the most common once.
| namespace Microsoft.PowerShell.Commands.Utility | ||
| { | ||
| /// <summary> | ||
| /// Group-Object implementation. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
|
||
| if (PropertyName == null) | ||
| { | ||
| if (_inputObjects.Count > 0) |
This comment has been minimized.
This comment has been minimized.
PaulHigin
Aug 30, 2018
Contributor
I think you can just check count once and early out or just skip the if/else processing.
This comment has been minimized.
This comment has been minimized.
| private readonly PowerShell _powerShell; | ||
|
|
||
| public TypeInferenceContext() | ||
| : this(PowerShell.Create(RunspaceMode.CurrentRunspace)) | ||
| { | ||
| _ownsPowerShell = true; |
This comment has been minimized.
This comment has been minimized.
PaulHigin
Aug 30, 2018
Contributor
This doesn't look right to me. This class can own PowerShell/Runspace only if PowerShell is created with Runspace.NewRunspace mode. Otherwise you are disposing a runspace created by someone else (and possibly the thread default runspace).
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
powercode
added some commits
Aug 31, 2018
This comment has been minimized.
This comment has been minimized.
|
@PaulHigin Thx for the review. |
iSazonov
reviewed
Aug 31, 2018
| CommandAst commandAst, | ||
| IDictionary fakeBoundParameters) | ||
| { | ||
| var res = new List<CompletionResult>(10); |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
powercode
Aug 31, 2018
Author
Collaborator
Then you end up with races if used from multiple runspaces.
This comment has been minimized.
This comment has been minimized.
iSazonov
Aug 31, 2018
Collaborator
The list looks as a const for current session. Do I skip something?
This comment has been minimized.
This comment has been minimized.
powercode
Aug 31, 2018
Author
Collaborator
Yes, it is captured by the local function AddMatching.
So the result is depending on wordToComplete.
This comment has been minimized.
This comment has been minimized.
iSazonov
Aug 31, 2018
Collaborator
I see now. :-) It is Friday. I can not catch up with my idea :-)
I am looking at 'new CompletionResult()'. If they isn't modifed later we could make it static and only reference in target List?
| /// </summary> | ||
| [Parameter(Position = 0)] | ||
| [ArgumentCompleter(typeof(PropertyNameCompleter))] | ||
| public object PropertyName { get; set; } |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
powercode
Aug 31, 2018
Author
Collaborator
PropertyExpression has some extra semantics, handling hashtables like @{n="x";ex={2,3,4}}.
I looked at it, but was not convinced it was a fit. But there's definitely an overlap. Input here is most welcome.
iSazonov
reviewed
Aug 31, 2018
| case "\r": return "`r"; | ||
| case "\n": return "`n"; | ||
| case "\r\n": return "`r`n"; | ||
| default: return Environment.NewLine.Replace("\r", "`r").Replace("\n", "`n"); |
This comment has been minimized.
This comment has been minimized.
iSazonov
Aug 31, 2018
Collaborator
Seems the switch is superfluous. We could leave only
``'c#
return Environment.NewLine.Replace("\r", "r").Replace("\n", "n");
This comment has been minimized.
This comment has been minimized.
powercode
Aug 31, 2018
Author
Collaborator
Yes it could. I wrote it the way I did to reduce allocated objects. May not be worth it.
The string literals will be loaded from metadata and interned. There will only exist one instance of them. The default line will create two new strings each time.
This comment has been minimized.
This comment has been minimized.
iSazonov
Aug 31, 2018
Collaborator
Environment.NewLine is static. So I think we should use the usual method
public static string NewLineText
{
get
{
#if UNIX
return "`n"
#else
return "`r`n"
#endif
}
}
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
powercode
reviewed
Aug 31, 2018
|
A general question is how we should handle |
| CommandAst commandAst, | ||
| IDictionary fakeBoundParameters) | ||
| { | ||
| var res = new List<CompletionResult>(10); |
This comment has been minimized.
This comment has been minimized.
powercode
Aug 31, 2018
Author
Collaborator
Yes, it is captured by the local function AddMatching.
So the result is depending on wordToComplete.
This comment has been minimized.
This comment has been minimized.
|
Ex.: $null,$null | % { 1 }
1
1
[System.Management.Automation.Internal.AutomationNull]::Value,[System.Management.Automation.Internal.AutomationNull]::Value | % { 1 } |
This comment has been minimized.
This comment has been minimized.
|
Should it be named |
PaulHigin
approved these changes
Aug 31, 2018
|
LGTM |
This comment has been minimized.
This comment has been minimized.
|
I'd be inclined to call it Join-String instead, since nothing of the original objects are preserved (counter to things like Group-Object or Select-Object) |
powercode
added some commits
Aug 31, 2018
This comment has been minimized.
This comment has been minimized.
|
@BrucePay can you please take a look again at this PR? Thanks. |
iSazonov
reviewed
Sep 6, 2018
|
|
||
| /// <summary> | ||
| /// Gets or sets a format string that is applied to each input object. | ||
| /// </summary> |
This comment has been minimized.
This comment has been minimized.
iSazonov
Sep 6, 2018
Collaborator
I'd expand the comment with what is the format and/or add a link on docs.
| [OutputType(typeof(string))] | ||
| public sealed class JoinStringCommand : PSCmdlet | ||
| { | ||
| /// <summary>A bigger default to not get re-allocations in common use cases.</summary> |
This comment has been minimized.
This comment has been minimized.
iSazonov
Sep 6, 2018
Collaborator
Please use file pattern:
/// <Summary>
/// A bigger default to not get re-allocations in common use cases.
/// </Summary> | { | ||
| _outputBuilder.Append(_quoteChar); | ||
| _outputBuilder.Append(stringValue); | ||
| _outputBuilder.Append(_quoteChar); |
This comment has been minimized.
This comment has been minimized.
iSazonov
Sep 6, 2018
Collaborator
We could use
_outputBuilder.Append(_quoteChar).Append(stringValue).(_quoteChar); | } | ||
| else | ||
| { | ||
| _outputBuilder.AppendFormat(CultureInfo.CurrentCulture, FormatString, stringValue); |
This comment has been minimized.
This comment has been minimized.
iSazonov
Sep 6, 2018
Collaborator
I'd want to get confirmation that this should be CurrentCulture not InvariantCulture.
And maybe discuss -FormatCulture parameter.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
mklement0
Sep 6, 2018
Contributor
PowerShell consistently uses the invariant culture in to/from-string conversions, so it should probably be InvariantCulture, at least by default, perhaps with an optional -UseCulture switch (analogous to the *-Csv cmdlets).
This comment has been minimized.
This comment has been minimized.
BrucePay
Sep 15, 2018
Collaborator
As a guideline, we use InvariantCulture when manipulating data so that the behaviour of the code is predictable (invariant) across locales and we use current culture when presenting to the user. This cmdlet could be used in both scenarios but I expect it will be used more for data processing so I suggest defaulting to InvariantCulture with an option to override for current culture.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
iSazonov
Oct 28, 2018
•
Collaborator
gcm -ParameterName Culture
CommandType Name Version Source
----------- ---- ------- ------
Cmdlet Compare-Object 3.1.0.0 Microsoft.PowerShell.Utility
Cmdlet Group-Object 3.1.0.0 Microsoft.PowerShell.Utility
Cmdlet New-PSSessionOption 6.0.0.0 Microsoft.PowerShell.Core
Cmdlet Sort-Object 3.1.0.0 Microsoft.PowerShell.UtilityWe already have -Culture parameter. Make sense re-use the name?
Or -UseCurrentCulture? Looks as limited option.
This comment has been minimized.
This comment has been minimized.
mklement0
Oct 28, 2018
Contributor
I think -UseCulture is the correct name, after all:
-
The
-Culture <string>parameters you reference:- require an argument, namely a given culture
- are used with cmdlets that are current-culture-sensitive by default, to allow opt-in to a different culture.
-
The
-UseCultureswitch, by contrast:- is used with cmdlets that are culture-invariant by default, to allow opt-in to the current culture (currently applies only to the
*-Csvcmdlets).
- is used with cmdlets that are culture-invariant by default, to allow opt-in to the current culture (currently applies only to the
The latter is what is being implemented here.
This comment has been minimized.
This comment has been minimized.
| get | ||
| { | ||
| #if UNIX | ||
| return Platform.IsMacOS ? "`r" : "`n"; |
This comment has been minimized.
This comment has been minimized.
|
|
||
| It "Should be called using an object as piped without error with no switches" { | ||
| {$testObject | Join-String } | Should -Not -Throw | ||
| } |
This comment has been minimized.
This comment has been minimized.
daxian-dbw
self-assigned this
Sep 20, 2018
daxian-dbw
added
the
Documentation Needed
label
Oct 9, 2018
daxian-dbw
closed this
Oct 9, 2018
daxian-dbw
reopened this
Oct 9, 2018
joeyaiello
referenced this pull request
Oct 15, 2018
Closed
Docs needed for 'Join-String cmdlet for creating text from pipeline input' #3026
joeyaiello
removed
Documentation Needed
labels
Oct 15, 2018
This comment has been minimized.
This comment has been minimized.
|
@powercode Looks like this PR still needs a couple of minor updates that are mentioned in @iSazonov's review; especially InvariantCulture issue. Otherwise it seems close to being done. |
This comment has been minimized.
This comment has been minimized.
|
Sorry - been a bit preoccupied. |
JamesWTruher
approved these changes
Oct 25, 2018
This comment has been minimized.
This comment has been minimized.
|
I just realized that when joining dates, I always get the "US", Invariant formatting, which is literally never the output a Swede would want. To get the I will add a new commit with this change - feel free to ignore it if you don't think it should go with this PR. |
powercode
force-pushed the
powercode:JoinObject
branch
to
937d9b8
Oct 27, 2018
sdwheeler
referenced this pull request
Oct 29, 2018
Merged
Documentation for join-string cmdlet. #3195
daxian-dbw
approved these changes
Nov 14, 2018
This comment has been minimized.
This comment has been minimized.
|
The last commit ( |
daxian-dbw
added
Documentation Needed
CL-General
labels
Nov 14, 2018
daxian-dbw
merged commit 877b9a9
into
PowerShell:master
Nov 14, 2018
6 of 7 checks passed
This comment has been minimized.
This comment has been minimized.
|
What is current policy: don't merge until doc issue is created? |
powercode commentedAug 29, 2018
•
edited by daxian-dbw
PR Summary
A join-object cmdlet that joins pipeline input to text.
#6697
As requested by @BrucePay.
Final syntax of the cmdlet:
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:to the beginning of the title and remove the prefix when the PR is ready.[feature]if the change is significant or affects feature tests