★ wanayoo — archive 1999 https://github.com/PowerShell/PowerShell/pull/7660Nouvelle recherche | Portail wanayoo
Skip to content
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

Join-String cmdlet for creating text from pipeline input #7660

Merged
merged 24 commits into from Nov 14, 2018

Conversation

@powercode
Copy link
Collaborator

commented Aug 29, 2018

PR Summary

A join-object cmdlet that joins pipeline input to text.

#6697

As requested by @BrucePay.

Final syntax of the cmdlet:

Join-String [[-Property] <pspropertyexpression>] [[-Separator] <string>] [-OutputPrefix <string>] [-OutputSuffix <string>] [-UseCulture] [-InputObject <psobject>] [<CommonParameters>]

Join-String [[-Property] <pspropertyexpression>] [[-Separator] <string>] [-OutputPrefix <string>] [-OutputSuffix <string>] [-SingleQuote] [-UseCulture] [-InputObject <psobject>] [<CommonParameters>]

Join-String [[-Property] <pspropertyexpression>] [[-Separator] <string>] [-OutputPrefix <string>] [-OutputSuffix <string>] [-DoubleQuote] [-UseCulture] [-InputObject <psobject>] [<CommonParameters>]

Join-String [[-Property] <pspropertyexpression>] [[-Separator] <string>] [-OutputPrefix <string>] [-OutputSuffix <string>] [-FormatString <string>] [-UseCulture] [-InputObject <psobject>] [<CommonParameters>]
1..3 | Join-String -OutputPrefix "A " -OutputSuffix " B" -Separator "," -SingleQuote | Should -BeExactly "A '1','2','3' B" 
gci | Join-String Name -Separator "," -DoubleQuote
gci | Join-String {$_.Name * 2} "; "

PR Checklist

powercode added some commits Aug 29, 2018

@iSazonov

This comment has been minimized.

Copy link
Collaborator

commented Aug 30, 2018

'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

@powercode

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 30, 2018

It is intended to be the pipeline equivalent of the -join operator. @BrucePay suggested the name Join-Object in the issue. Convert would imply for me that it converted each item to a string. This cmdlet join them together with a delimiter.

@RichardSiddaway

This comment has been minimized.

Copy link

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

@powercode

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 30, 2018

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.

/// </summary>
[Cmdlet(VerbsCommon.Join, "Object", RemotingCapability = RemotingCapability.None, DefaultParameterSetName = "default")]
[OutputType(typeof(string))]
public class JoinObjectCommand : PSCmdlet

This comment has been minimized.

Copy link
@PaulHigin

PaulHigin Aug 30, 2018

Contributor

I think this should be public sealed class.

This comment has been minimized.

Copy link
@powercode

powercode Aug 31, 2018

Author Collaborator

yes

public class JoinObjectCommand : PSCmdlet
{
// ReSharper disable once CollectionNeverQueried.Local
private readonly List<PSObject> _inputObjects = new List<PSObject>(50);

This comment has been minimized.

Copy link
@PaulHigin

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.

Copy link
@powercode

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.

Copy link
@PaulHigin

PaulHigin Aug 30, 2018

Contributor

Join-Object implementation

This comment has been minimized.

Copy link
@powercode

powercode Aug 31, 2018

Author Collaborator

oops :)


if (PropertyName == null)
{
if (_inputObjects.Count > 0)

This comment has been minimized.

Copy link
@PaulHigin

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.

Copy link
@powercode

powercode Aug 31, 2018

Author Collaborator

Yes, much better!

Show resolved Hide resolved ...m.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs
Show resolved Hide resolved ...m.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs Outdated
Show resolved Hide resolved ...m.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs
private readonly PowerShell _powerShell;

public TypeInferenceContext()
: this(PowerShell.Create(RunspaceMode.CurrentRunspace))
{
_ownsPowerShell = true;

This comment has been minimized.

Copy link
@PaulHigin

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.

Copy link
@powercode

powercode Aug 31, 2018

Author Collaborator

Good catch!

This comment has been minimized.

Copy link
@powercode

powercode Aug 31, 2018

Author Collaborator

Reverting the whole file.

@powercode

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 31, 2018

@PaulHigin Thx for the review.

CommandAst commandAst,
IDictionary fakeBoundParameters)
{
var res = new List<CompletionResult>(10);

This comment has been minimized.

Copy link
@iSazonov

iSazonov Aug 31, 2018

Collaborator

Can it be static?

This comment has been minimized.

Copy link
@powercode

powercode Aug 31, 2018

Author Collaborator

Then you end up with races if used from multiple runspaces.

This comment has been minimized.

Copy link
@iSazonov

iSazonov Aug 31, 2018

Collaborator

The list looks as a const for current session. Do I skip something?

This comment has been minimized.

Copy link
@powercode

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.

Copy link
@iSazonov

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?

Show resolved Hide resolved src/Microsoft.PowerShell.Commands.Utility/commands/utility/join-object.cs Outdated
Show resolved Hide resolved test/powershell/Modules/Microsoft.PowerShell.Utility/Join-Object.Tests.ps1 Outdated
/// </summary>
[Parameter(Position = 0)]
[ArgumentCompleter(typeof(PropertyNameCompleter))]
public object PropertyName { get; set; }

This comment has been minimized.

Copy link
@iSazonov

iSazonov Aug 31, 2018

Collaborator

Can we use PSPropertyExpression type like in #6934?

This comment has been minimized.

Copy link
@powercode

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.

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.

Copy link
@iSazonov

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.

Copy link
@powercode

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.

Copy link
@iSazonov

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.

Copy link
@powercode

powercode Aug 31, 2018

Author Collaborator

Good point!

This comment has been minimized.

Copy link
@powercode

powercode Aug 31, 2018

Author Collaborator

But I think MacOS uses `r

This comment has been minimized.

Copy link
@iSazonov

iSazonov Sep 3, 2018

Collaborator

No, only very old MacOs versions. See my comment below.

@powercode
Copy link
Collaborator Author

left a comment

A general question is how we should handle null and AutomationNull.Value in the input?

CommandAst commandAst,
IDictionary fakeBoundParameters)
{
var res = new List<CompletionResult>(10);

This comment has been minimized.

Copy link
@powercode

powercode Aug 31, 2018

Author Collaborator

Yes, it is captured by the local function AddMatching.
So the result is depending on wordToComplete.

@iSazonov

This comment has been minimized.

Copy link
Collaborator

commented Aug 31, 2018

AutomationNull.Value is ignored by default in pipes

/// AutomationNull.Value is ignored

Ex.:

$null,$null | % { 1 }
1
1

[System.Management.Automation.Internal.AutomationNull]::Value,[System.Management.Automation.Internal.AutomationNull]::Value | % { 1 }
@powercode

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 31, 2018

Should it be named Join-String instead?

@PaulHigin
Copy link
Contributor

left a comment

LGTM

@vexx32

This comment has been minimized.

Copy link
Contributor

commented Aug 31, 2018

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

Renaming Join-Object => Join-String.
PreScript to Prefix
PostScript to Suffix.
PropertyName to Property.
@anmenaga

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2018

@BrucePay can you please take a look again at this PR? Thanks.


/// <summary>
/// Gets or sets a format string that is applied to each input object.
/// </summary>

This comment has been minimized.

Copy link
@iSazonov

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.

Copy link
@iSazonov

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.

Copy link
@iSazonov

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.

Copy link
@iSazonov

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.

Copy link
@powercode

powercode Sep 6, 2018

Author Collaborator

Same here. Now sure what is right here.

This comment has been minimized.

Copy link
@mklement0

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.

Copy link
@BrucePay

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.

Copy link
@powercode

powercode Oct 27, 2018

Author Collaborator

Adding UseCulture switch as suggested.

This comment has been minimized.

Copy link
@iSazonov

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

We already have -Culture parameter. Make sense re-use the name?
Or -UseCurrentCulture? Looks as limited option.

This comment has been minimized.

Copy link
@mklement0

mklement0 Oct 28, 2018

Contributor

@iSazonov:

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 -UseCulture switch, 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 *-Csv cmdlets).

The latter is what is being implemented here.

This comment has been minimized.

Copy link
@iSazonov

iSazonov Oct 29, 2018

Collaborator

@mklement0 Thanks! We need discuss Select-String too.

get
{
#if UNIX
return Platform.IsMacOS ? "`r" : "`n";

This comment has been minimized.

Copy link
@iSazonov

iSazonov Sep 6, 2018

Collaborator

Please address the comment.


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.

Copy link
@iSazonov

iSazonov Sep 6, 2018

Collaborator

We could remove the test - any test below do the test.

@BrucePay
Copy link
Collaborator

left a comment

LGTM

@anmenaga

This comment has been minimized.

Copy link
Contributor

commented Oct 23, 2018

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

@powercode

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 24, 2018

Sorry - been a bit preoccupied.

@powercode

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 27, 2018

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 UseCulture switch to have effect, I need to do some minor changes in how LanguagePrimitives makes the conversion to strings, and provide an overload that accepts an IFormatProvider.

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 powercode force-pushed the powercode:JoinObject branch to 937d9b8 Oct 27, 2018

@sdwheeler sdwheeler referenced this pull request Oct 29, 2018

Merged

Documentation for join-string cmdlet. #3195

2 of 9 tasks complete
@daxian-dbw

This comment has been minimized.

Copy link
Member

commented Nov 14, 2018

The last commit ("Make TryConvertTo<string> respect the passed formatting provider.") looks good to me.

@daxian-dbw daxian-dbw merged commit 877b9a9 into PowerShell:master Nov 14, 2018

6 of 7 checks passed

CodeFactor 2 issues fixed. 5 issues found.
Details
PowerShell-CI-linux #PR-7660-20181027.03 succeeded
Details
PowerShell-CI-macos #PR-7660-20181027.03 succeeded
Details
PowerShell-CI-spelling #PR-7660-20181027.03 succeeded
Details
PowerShell-CI-windows #PR-7660-20181027.03 succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla All CLA requirements met.
Details
@iSazonov

This comment has been minimized.

Copy link
Collaborator

commented Nov 15, 2018

What is current policy: don't merge until doc issue is created?

iSazonov added a commit to iSazonov/PowerShell that referenced this pull request Nov 29, 2018

Add cmdlet 'Join-String' for creating text from pipeline input (Power…
…Shell#7660)

The cmdlet syntax is as follows:
```
Join-String [[-Property] <pspropertyexpression>] [[-Separator] <string>] [-OutputPrefix <string>] [-OutputSuffix <string>] [-UseCulture] [-InputObject <psobject>] [<CommonParameters>]

Join-String [[-Property] <pspropertyexpression>] [[-Separator] <string>] [-OutputPrefix <string>] [-OutputSuffix <string>] [-SingleQuote] [-UseCulture] [-InputObject <psobject>] [<CommonParameters>]

Join-String [[-Property] <pspropertyexpression>] [[-Separator] <string>] [-OutputPrefix <string>] [-OutputSuffix <string>] [-DoubleQuote] [-UseCulture] [-InputObject <psobject>] [<CommonParameters>]

Join-String [[-Property] <pspropertyexpression>] [[-Separator] <string>] [-OutputPrefix <string>] [-OutputSuffix <string>] [-FormatString <string>] [-UseCulture] [-InputObject <psobject>] [<CommonParameters>]
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.