pnp
Remove `chatUtil.convertParticipantStringToArray` from code. Only split by comma.
Decision
We decided that we won't introduce splitting strings by space in powershell. Instead we advise people to use quotes. The existing splitter needs to be removed.
The Idea that sparked the discussion (for reference sake)
The idea is to refactor the chatUtil.convertParticipantStringToArray function to the formatting util class and rename it to splitOptionsStringToArray. This mainly so that this function could be used in multiple places. Any command that uses .split(',') on an option parameter qualifies for this.
Split doesn't work in PowerShell if the user runs the command as --option variable1,variable2 (without the quotes ""). This is because commas are reserved characters. PowerShell will convert it to an array and concatenate the values with a space. Original comment by @martinlingstuyl https://github.com/pnp/cli-microsoft365/pull/3315#discussion_r876957550
Commands to adjust:
| Command | Comma-separated option(s) |
|---|---|
| aad app add | redirectUris, apisDelegated, apisApplication |
| aad app set | redirectUris, redirectUrisToRemove |
| aad o365group add | owners, members |
| aad o365group set | owners, members |
| aad siteclassification enable | classifications |
| aad siteclassification set | classifications |
| aad user get | properties |
| aad user list | properties |
| graph schemaextension add | targetTypes |
| graph schemaextension set | targetTypes |
| graph subscription add | changeType |
| outlook mail send | to |
| planner task add | assignedToUserIds, assignedToUserNames, appliedCategories |
| planner task set | assignedToUserIds, assignedToUserNames, appliedCategories |
| search externalconnection add | authorizedAppIds |
| spo search | selectProperties |
| spo hubsite rights grant | principals |
| spo hubsite rights revoke | principals |
| spo list get | properties |
| spo listitem get | properties |
| spo listitem list | fields |
| spo mail send | to, cc, bcc |
| spo page header set | authors |
| spo site add | owners |
| spo site classic set | owners |
| spo site ensure | owners |
| spo site set | owners |
| spo sitedesign add | siteScripts |
| spo sitedesign rights grant | principals |
| spo sitedesign rights revoke | principals |
| spo sitedesign set | siteScripts |
| teams chat get | participants |
| teams chat message send | userEmails |
| teams team clone | partsToClone |
What if we rename it to 'splitOptionsStringToArray'?
Good suggestion. I have adjusted the specs.
Any command that uses .split(',') on an option parameter qualifies for this.
I don't quite agree with that. If our code expects a comma-separated string, then splitting on a space might introduce undesired behavior where people might specify a value with a space like a person's name. I'd say that we keep both cases separate and only use the space or comma separation behavior where we explicitly expect it and mention clearly in the docs.
I agree Waldek, we should only implement this where the values can't contain spaces. Things like emailadresses or values that are known upfront.
We should carefully go through the list and see on what commands this applies!
Maybe we should backtrack here and discuss whether we want to support this at all.
The point is: PoSh behavior is not userfriendly when you use an option this way. (Eg: comma separated without quotes)
So what do we do: so we try to help the user here, with the possible side effect of splitting a string the wrong way?
Isn't an easier way about it to use quotes with all values? I believe we're already doing this in all our examples, correct?
It's okay by me. Integration with PoSh is different then when using a PoSh module. We'll just have to accept that..