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 — Join-String cmdlet for creating text from pipeline input by powercode · Pull Request #7660 · PowerShell/PowerShell

Conversation

@powercode

@powercode Staffan Gustafsson (powercode) commented Aug 29, 2018 •
edited by daxian-dbw
Loading

Uh oh!

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

Copy link
Copy Markdown
Collaborator

PR Summary

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

#6697

As requested by Bruce Payette (@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

  • 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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
rkeithhill reacted with hooray emoji rkeithhill reacted with heart emoji
All reactions
  • 🎉 1 reaction
  • ❤️ 1 reaction
Staffan Gustafsson (powercode) added 4 commits August 30, 2018 00:11
@powercode
Fix disposing of powershell in TypeInferenceContext if it was created…
6997286 
… internally.
@powercode
Adding a PropertyNameCompleter custom completer.
1977dc2
@powercode
Adding Join-Object command
ba17c6c
@powercode
Adding tests
b34453e
@powercode
Staffan Gustafsson (powercode) requested review from Bruce Payette (BrucePay), James Truher (JamesWTruher), Paul Higinbotham (PaulHigin), Dan Travison (dantraMSFT) and Dongbo Wang (daxian-dbw) as code owners August 29, 2018 23:33
@iSazonov

Ilya (iSazonov) commented Aug 30, 2018 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Collaborator

'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
All reactions

Sorry, something went wrong.

Uh oh!

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

Staffan Gustafsson (powercode) commented Aug 30, 2018

Copy link
Copy Markdown
Collaborator Author

It is intended to be the pipeline equivalent of the -join operator. Bruce Payette (@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.
All reactions

Sorry, something went wrong.

Uh oh!

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

RichardSiddaway commented Aug 30, 2018

Copy link
Copy Markdown

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

Staffan Gustafsson (powercode) commented Aug 30, 2018

Copy link
Copy Markdown
Collaborator Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
PaulHigin
Paul Higinbotham (PaulHigin) suggested changes Aug 30, 2018
View reviewed changes
Comment thread src/Microsoft.PowerShell.Commands.Utility/commands/utility/join-object.cs Outdated
/// </summary>
[Cmdlet(VerbsCommon.Join, "Object", RemotingCapability = RemotingCapability.None, DefaultParameterSetName = "default")]
[OutputType(typeof(string))]
public class JoinObjectCommand : PSCmdlet

@PaulHigin Paul Higinbotham (PaulHigin) Aug 30, 2018

Copy link
Copy Markdown
Contributor

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.

I think this should be public sealed class.

Sorry, something went wrong.

Uh oh!

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

@powercode Staffan Gustafsson (powercode) Aug 31, 2018

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

yes

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/Microsoft.PowerShell.Commands.Utility/commands/utility/join-object.cs Outdated
public class JoinObjectCommand : PSCmdlet
{
// ReSharper disable once CollectionNeverQueried.Local
private readonly List<PSObject> _inputObjects = new List<PSObject>(50);

@PaulHigin Paul Higinbotham (PaulHigin) Aug 30, 2018

Copy link
Copy Markdown
Contributor

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.

I normally don't see List size initialization unless the size is known. I am curious why you initialize the size to this value?

Sorry, something went wrong.

Uh oh!

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

@powercode Staffan Gustafsson (powercode) Aug 31, 2018

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/Microsoft.PowerShell.Commands.Utility/commands/utility/join-object.cs Outdated
namespace Microsoft.PowerShell.Commands.Utility
{
/// <summary>
/// Group-Object implementation.

@PaulHigin Paul Higinbotham (PaulHigin) Aug 30, 2018

Copy link
Copy Markdown
Contributor

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.

Join-Object implementation

Sorry, something went wrong.

Uh oh!

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

@powercode Staffan Gustafsson (powercode) Aug 31, 2018

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

oops :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/Microsoft.PowerShell.Commands.Utility/commands/utility/join-object.cs Outdated

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

@PaulHigin Paul Higinbotham (PaulHigin) Aug 30, 2018

Copy link
Copy Markdown
Contributor

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.

I think you can just check count once and early out or just skip the if/else processing.

Sorry, something went wrong.

Uh oh!

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

@powercode Staffan Gustafsson (powercode) Aug 31, 2018

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

Yes, much better!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/System.Management.Automation/engine/parser/TypeInferenceVisitor.cs Outdated
public TypeInferenceContext()
: this(PowerShell.Create(RunspaceMode.CurrentRunspace))
{
_ownsPowerShell = true;

@PaulHigin Paul Higinbotham (PaulHigin) Aug 30, 2018

Copy link
Copy Markdown
Contributor

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.

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

Sorry, something went wrong.

Uh oh!

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

@powercode Staffan Gustafsson (powercode) Aug 31, 2018

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

Good catch!

Sorry, something went wrong.

Uh oh!

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

@powercode Staffan Gustafsson (powercode) Aug 31, 2018

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

Reverting the whole file.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Staffan Gustafsson (powercode) added 2 commits August 31, 2018 08:03
@powercode
Reverting incorrect dispose in TypeInferenceVisitor
cf646c1
@powercode
Addressing Pauls review comments.
32fd136
@powercode

Staffan Gustafsson (powercode) commented Aug 31, 2018

Copy link
Copy Markdown
Collaborator Author

Paul Higinbotham (@PaulHigin) Thx for the review.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@powercode
Early out when inputobject count == 0.
acbdbf9
iSazonov
Ilya (iSazonov) reviewed Aug 31, 2018
View reviewed changes
Comment thread src/Microsoft.PowerShell.Commands.Utility/commands/utility/join-object.cs Outdated
CommandAst commandAst,
IDictionary fakeBoundParameters)
{
var res = new List<CompletionResult>(10);

@iSazonov Ilya (iSazonov) Aug 31, 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.

Can it be static?

Sorry, something went wrong.

Uh oh!

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

@powercode Staffan Gustafsson (powercode) Aug 31, 2018

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

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

Sorry, something went wrong.

Uh oh!

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

@iSazonov Ilya (iSazonov) Aug 31, 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.

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

Sorry, something went wrong.

Uh oh!

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

@powercode Staffan Gustafsson (powercode) Aug 31, 2018

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

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

Sorry, something went wrong.

Uh oh!

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

@iSazonov Ilya (iSazonov) Aug 31, 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.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/Microsoft.PowerShell.Commands.Utility/commands/utility/join-object.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread test/powershell/Modules/Microsoft.PowerShell.Utility/Join-Object.Tests.ps1 Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/Microsoft.PowerShell.Commands.Utility/commands/utility/join-object.cs Outdated
/// </summary>
[Parameter(Position = 0)]
[ArgumentCompleter(typeof(PropertyNameCompleter))]
public object PropertyName { get; set; }

@iSazonov Ilya (iSazonov) Aug 31, 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.

Can we use PSPropertyExpression type like in #6934?

Sorry, something went wrong.

Uh oh!

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

@powercode Staffan Gustafsson (powercode) Aug 31, 2018

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
iSazonov
Ilya (iSazonov) reviewed Aug 31, 2018
View reviewed changes
Comment thread src/Microsoft.PowerShell.Commands.Utility/commands/utility/join-object.cs Outdated
case "\r": return "`r";
case "\n": return "`n";
case "\r\n": return "`r`n";
default: return Environment.NewLine.Replace("\r", "`r").Replace("\n", "`n");

@iSazonov Ilya (iSazonov) Aug 31, 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.

Seems the switch is superfluous. We could leave only
``'c#
return Environment.NewLine.Replace("\r", "r").Replace("\n", "n");

Sorry, something went wrong.

Uh oh!

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

@powercode Staffan Gustafsson (powercode) Aug 31, 2018

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

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.

Sorry, something went wrong.

Uh oh!

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

@iSazonov Ilya (iSazonov) Aug 31, 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.

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

Sorry, something went wrong.

Uh oh!

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

@powercode Staffan Gustafsson (powercode) Aug 31, 2018

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

Good point!

Sorry, something went wrong.

Uh oh!

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

@powercode Staffan Gustafsson (powercode) Aug 31, 2018

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

But I think MacOS uses `r

Sorry, something went wrong.

Uh oh!

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

@iSazonov Ilya (iSazonov) Sep 3, 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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
powercode
Staffan Gustafsson (powercode) commented Aug 31, 2018
View reviewed changes

@powercode Staffan Gustafsson (powercode) left a comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/Microsoft.PowerShell.Commands.Utility/commands/utility/join-object.cs Outdated
CommandAst commandAst,
IDictionary fakeBoundParameters)
{
var res = new List<CompletionResult>(10);

@powercode Staffan Gustafsson (powercode) Aug 31, 2018

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

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

Sorry, something went wrong.

Uh oh!

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

Ilya (iSazonov) commented Aug 31, 2018

Copy link
Copy Markdown
Collaborator

AutomationNull.Value is ignored by default in pipes

PowerShell/src/System.Management.Automation/engine/Pipe.cs

Line 380 in 4fc784e

/// AutomationNull.Value is ignored

Ex.:

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

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

Sorry, something went wrong.

Uh oh!

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

Staffan Gustafsson (powercode) commented Aug 31, 2018

Copy link
Copy Markdown
Collaborator Author

Should it be named Join-String instead?
iSazonov, rkeithhill, vexx32, BrucePay, and dragonwolf83 reacted with thumbs up emoji
All reactions
  • 👍 5 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
PaulHigin
Paul Higinbotham (PaulHigin) approved these changes Aug 31, 2018
View reviewed changes

@PaulHigin Paul Higinbotham (PaulHigin) left a comment

Copy link
Copy Markdown
Contributor

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.

LGTM

Sorry, something went wrong.

Uh oh!

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

Rain Sallow (/u/ta11ow) (vexx32) commented Aug 31, 2018

Copy link
Copy Markdown
Collaborator

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)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Staffan Gustafsson (powercode) added 5 commits August 31, 2018 18:46
@powercode
Fixing missing TypeInferenceVisitor Dispose change.
bda1740
@powercode
Renaming Join-Object => Join-String.
2995f92 
PreScript to Prefix
PostScript to Suffix.
PropertyName to Property.
@powercode
newline at eof.
2a0b56a
@powercode
Renaming files *-object -> *-string
c68da1f
@powercode
platform specific newline completion text.
68c0f57
@anmenaga

Andrew (anmenaga) commented Sep 5, 2018

Copy link
Copy Markdown

Bruce Payette (@BrucePay) can you please take a look again at this PR? Thanks.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
iSazonov
Ilya (iSazonov) reviewed Sep 6, 2018
View reviewed changes
Comment thread src/Microsoft.PowerShell.Commands.Utility/commands/utility/join-string.cs

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

@iSazonov Ilya (iSazonov) Sep 6, 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.

I'd expand the comment with what is the format and/or add a link on docs.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/Microsoft.PowerShell.Commands.Utility/commands/utility/join-string.cs
[OutputType(typeof(string))]
public sealed class JoinStringCommand : PSCmdlet
{
/// <summary>A bigger default to not get re-allocations in common use cases.</summary>

@iSazonov Ilya (iSazonov) Sep 6, 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 use file pattern:

/// <Summary>
/// A bigger default to not get re-allocations in common use cases.
/// </Summary>

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/Microsoft.PowerShell.Commands.Utility/commands/utility/join-string.cs
{
_outputBuilder.Append(_quoteChar);
_outputBuilder.Append(stringValue);
_outputBuilder.Append(_quoteChar);

@iSazonov Ilya (iSazonov) Sep 6, 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.

We could use

_outputBuilder.Append(_quoteChar).Append(stringValue).(_quoteChar);

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
powercode reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread src/Microsoft.PowerShell.Commands.Utility/commands/utility/join-string.cs Outdated
}
else
{
_outputBuilder.AppendFormat(CultureInfo.CurrentCulture, FormatString, stringValue);

@iSazonov Ilya (iSazonov) Sep 6, 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.

I'd want to get confirmation that this should be CurrentCulture not InvariantCulture.
And maybe discuss -FormatCulture parameter.

Sorry, something went wrong.

Uh oh!

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

@powercode Staffan Gustafsson (powercode) Sep 6, 2018

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

Same here. Now sure what is right here.

Sorry, something went wrong.

Uh oh!

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

@mklement0 Michael Klement (mklement0) Sep 6, 2018

Copy link
Copy Markdown
Contributor

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.

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

Sorry, something went wrong.

Uh oh!

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

@BrucePay Bruce Payette (BrucePay) Sep 15, 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.

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.

Sorry, something went wrong.

Uh oh!

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

@powercode Staffan Gustafsson (powercode) Oct 27, 2018

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

Adding UseCulture switch as suggested.

Sorry, something went wrong.

Uh oh!

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

@iSazonov Ilya (iSazonov) Oct 28, 2018 •
edited
Loading

Uh oh!

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

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. 

 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.

Sorry, something went wrong.

Uh oh!

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

@mklement0 Michael Klement (mklement0) Oct 28, 2018

Copy link
Copy Markdown
Contributor

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.

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

Sorry, something went wrong.

Uh oh!

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

@iSazonov Ilya (iSazonov) Oct 29, 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.

Michael Klement (@mklement0) Thanks! We need discuss Select-String 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.Utility/commands/utility/join-string.cs Outdated
get
{
#if UNIX
return Platform.IsMacOS ? "`r" : "`n";

@iSazonov Ilya (iSazonov) Sep 6, 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 address the comment.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/powershell/Modules/Microsoft.PowerShell.Utility/Join-String.Tests.ps1 Outdated

It "Should be called using an object as piped without error with no switches" {
{$testObject | Join-String } | Should -Not -Throw
}

@iSazonov Ilya (iSazonov) Sep 6, 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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
BrucePay
Bruce Payette (BrucePay) approved these changes Sep 15, 2018
View reviewed changes

@BrucePay Bruce Payette (BrucePay) 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.

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@daxian-dbw Dongbo Wang (daxian-dbw) self-assigned this Sep 20, 2018
@daxian-dbw Dongbo Wang (daxian-dbw) added the Documentation Needed in this repo Documentation is needed in this repo label Oct 9, 2018
@daxian-dbw Dongbo Wang (daxian-dbw) closed this Oct 9, 2018
@daxian-dbw Dongbo Wang (daxian-dbw) reopened this Oct 9, 2018
@joeyaiello Joey Aiello (joeyaiello) mentioned this pull request Oct 15, 2018
Docs needed for 'Join-String cmdlet for creating text from pipeline input' MicrosoftDocs/PowerShell-Docs#3026
Closed
@joeyaiello Joey Aiello (joeyaiello) removed Documentation Needed in this repo Documentation is needed in this repo labels Oct 15, 2018
@powercode
Addressing Ilya's comments.
91d071e
@anmenaga

Andrew (anmenaga) commented Oct 23, 2018

Copy link
Copy Markdown

Staffan Gustafsson (@powercode) Looks like this PR still needs a couple of minor updates that are mentioned in Ilya (@iSazonov)'s review; especially InvariantCulture issue. Otherwise it seems close to being done.
powercode 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.
@powercode

Staffan Gustafsson (powercode) commented Oct 24, 2018

Copy link
Copy Markdown
Collaborator Author

Sorry - been a bit preoccupied.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
JamesWTruher
James Truher (JamesWTruher) approved these changes Oct 25, 2018
View reviewed changes
@powercode
Adding UseCulture switch to the Format parameter set.
adddfde
@powercode

Staffan Gustafsson (powercode) commented Oct 27, 2018 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Collaborator Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@powercode
Make TryConvertTo<string> respect the passed formatting provider.
937d9b8
@powercode
Staffan Gustafsson (powercode) force-pushed the JoinObject branch from d64cc84 to 937d9b8 Compare October 27, 2018 22:00
@sdwheeler Sean Wheeler (sdwheeler) mentioned this pull request Oct 29, 2018
Documentation for join-string cmdlet. MicrosoftDocs/PowerShell-Docs#3195
Merged
2 of 9 tasks
daxian-dbw
Dongbo Wang (daxian-dbw) approved these changes Nov 14, 2018
View reviewed changes
@daxian-dbw

Dongbo Wang (daxian-dbw) commented Nov 14, 2018

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@daxian-dbw Dongbo Wang (daxian-dbw) added Documentation Needed in this repo Documentation is needed in this repo CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log labels Nov 14, 2018
@daxian-dbw
Dongbo Wang (daxian-dbw) merged commit 877b9a9 into PowerShell:master Nov 14, 2018
@iSazonov

Ilya (iSazonov) commented Nov 15, 2018

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Ilya (iSazonov) pushed a commit to iSazonov/PowerShell that referenced this pull request Nov 29, 2018
@powercode @iSazonov
Add cmdlet 'Join-String' for creating text from pipeline input (Power…
eb1cbdb 
…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>]
```
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026
@powercode @daxian-dbw
Add cmdlet 'Join-String' for creating text from pipeline input (Power…
c1617c4 
…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

Reviewers

@iSazonov Ilya (iSazonov) iSazonov left review comments
@daxian-dbw Dongbo Wang (daxian-dbw) daxian-dbw approved these changes
@dantraMSFT Dan Travison (dantraMSFT) Awaiting requested review from dantraMSFT
+4 more reviewers
@mklement0 Michael Klement (mklement0) mklement0 left review comments
@BrucePay Bruce Payette (BrucePay) BrucePay approved these changes
@JamesWTruher James Truher (JamesWTruher) JamesWTruher approved these changes
@PaulHigin Paul Higinbotham (PaulHigin) PaulHigin approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

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

Assignees

@daxian-dbw Dongbo Wang (daxian-dbw)

Labels

CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log Documentation Needed in this repo Documentation is needed in this repo

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.

11 participants

@powercode @iSazonov @RichardSiddaway @vexx32 @anmenaga @daxian-dbw @mklement0 @BrucePay @JamesWTruher @PaulHigin @joeyaiello
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.