Small code refactor - avoid overwriting args and consider them as read only

Adam-it commented 1 year ago

🤔 Problem

We should not overwrite args and consider them always as read only. So this kind of code https://github.com/pnp/cli-microsoft365/blob/4b19747d800afa8595aa6d3e3929de6595b985b7/src/m365/spo/commands/list/list-roleassignment-add.ts#L172

should be refactored to use local variable

Commands

Currently I did not check if this problem is present also in other commands. This research could be done as part of this issue as well. But for sure till now we have this mistake done in

[ ] web roleassignment add
[ ] list roleassignment add

Adam-it commented 1 year ago

@pnp/cli-for-microsoft-365-maintainers any comments ?

Jwaegebaert commented 1 year ago

Great issue @Adam-it

I found the following commands aswell with the same issue. The numbers next to the commands are the line number.

SPFx

spfx-doctor 518

SPO

list-roleassignment-remove 151 & 155
listitem-roleassignment-remove 161 & 165
web-roleassignment-remove 109 & 113

Yammer

message-list 130

I've got more but these ones should be removed with the v6 release, so they could be ignored in this issue. What do you think?

Planner

bucket-add 114
bucket-get 138
bucket-list 105
bucket-remove 142
bucket-set 152
plan-get 129 & 133
task-add 192
task-get 136 & 148
task-list 123

SPO

field-get 107
field-get 118

Teams

team-archive 112
team-clone 133 & 139
team-remove 110
team-set 135 & 141
team-unarchive 96

waldekmastykarz commented 1 year ago

Very good proposal and I'm in favor of enforcing it. Could we spend a little time and see if we can report such assignments with eslint so that we've got something watching our back so that we won't do it again in the future?

Adam-it commented 1 year ago

Great work @Jwaegebaert 👍. Good suggestion @waldekmastykarz will try to have a check on that 🤔👍

Adam-it commented 1 year ago

I made a quick research on this one and it seems the general rule "no-param-reassign" will not do the trick to check only reassigning args. So Unless we want to make it a general rule not to allow to reassign any argument of any method (utils to correct ect.) we will need to create a custom rule I think (which we already have so I guess there is no issue in adding another one with a PR). @waldekmastykarz, @Jwaegebaert what do you think?

waldekmastykarz commented 1 year ago

On the surface, the no-param-reassign rule seems like a thing we could use. Have you tried enabling it on our code base and checking if it would yield and false positives? Ideally, we'd set it up to apply to just args and options, but perhaps it should apply to all function args at large, and we should explicitly disable it in specific cases where modifying function params is intentional.

Adam-it commented 1 year ago

On the surface, the no-param-reassign rule seems like a thing we could use. Have you tried enabling it on our code base and checking if it would yield and false positives? Ideally, we'd set it up to apply to just args and options, but perhaps it should apply to all function args at large, and we should explicitly disable it in specific cases where modifying function params is intentional.

I was thinking exactly the same. It's a good practice to have in general so why not have it globally and disable to some exceptions (which for sure we may have) I only enabled the rule and checked that we will get more or less 40 errors, but didn't spend much time on it as I wanted to hear your opinion on what would be better to do (global OOTB rule or our custom specific one 🤔). I will give it a double check and get back with some results

waldekmastykarz commented 1 year ago

From what I've seen in the rule options, we should set props to true in case we pass the whole args object as function parameter and would reassign options. Also, let's check that it catches args.options.option reassignment as well, when the whole args object is passed into a function, because the option is a child of a child of args that's passed into function.

Adam-it commented 1 year ago

@waldekmastykarz sorry for the late reply on this. So I checked this and the result is quite promising. it seems this would be our 'error' list to be corrected It seems that args.options.option reassignment is also considered. Also, many internal private methods inputs are overwritten like when setting webpart properties etc. this will need refactoring as well.

@pnp/cli-for-microsoft-365-maintainers what do you think? If we agree I could open a PR that adds this rule and resolves all of those problems in one go. TBH I am also not sure we should push it before v6 release as it could make many current PR's not valid anymore 🤔

D:\workspace\cli-microsoft365\src\Command.ts
  180:7  error  Assignment to property of function parameter 'args'            no-param-reassign
  445:7  error  Assignment to property of function parameter 'telemetryProps'  no-param-reassign
  453:7  error  Assignment to property of function parameter 'payload'         no-param-reassign
  472:9  error  Assignment to property of function parameter 'args'            no-param-reassign
  475:9  error  Assignment to property of function parameter 'args'            no-param-reassign

D:\workspace\cli-microsoft365\src\autocomplete.ts
  170:5  error  Assignment to property of function parameter 'functions'  no-param-reassign

D:\workspace\cli-microsoft365\src\cli\Cli.ts
  437:9   error  Assignment to function parameter 'logStatement'        no-param-reassign
  463:7   error  Assignment to function parameter 'logStatement'        no-param-reassign
  504:13  error  Assignment to function parameter 'logStatement'        no-param-reassign
  507:11  error  Assignment to function parameter 'logStatement'        no-param-reassign
  724:11  error  Assignment to property of function parameter 'object'  no-param-reassign
  816:9   error  Assignment to property of function parameter 'args'    no-param-reassign

D:\workspace\cli-microsoft365\src\m365\aad\commands\app\app-add.ts
  522:12  error  Assignment to property of function parameter 'manifest'  no-param-reassign
  541:7   error  Assignment to property of function parameter 'p'         no-param-reassign
  542:14  error  Assignment to property of function parameter 'p'         no-param-reassign
  571:9   error  Assignment to property of function parameter 'appInfo'   no-param-reassign
  643:16  error  Assignment to property of function parameter 'scope'     no-param-reassign
  644:16  error  Assignment to property of function parameter 'scope'     no-param-reassign
  674:16  error  Assignment to property of function parameter 'role'      no-param-reassign
  882:11  error  Assignment to property of function parameter 'appInfo'   no-param-reassign

D:\workspace\cli-microsoft365\src\m365\aad\commands\app\app-role-remove.ts
  161:5  error  Assignment to property of function parameter 'aadApp'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\aad\commands\group\group-list.ts
  63:13  error  Assignment to property of function parameter 'group'  no-param-reassign
  66:13  error  Assignment to property of function parameter 'group'  no-param-reassign
  69:13  error  Assignment to property of function parameter 'group'  no-param-reassign
  72:13  error  Assignment to property of function parameter 'group'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\aad\commands\o365group\o365group-add.ts
  296:17  error  Assignment to function parameter 'retryLeft'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\aad\commands\o365group\o365group-set.ts
  273:15  error  Assignment to function parameter 'retryLeft'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\aad\commands\o365group\o365group-user-list.ts
  102:11  error  Assignment to property of function parameter 'user'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\aad\commands\o365group\o365group-user-set.ts
  188:11  error  Assignment to property of function parameter 'user'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\app\commands\permission\permission-list.ts
  209:13  error  Assignment to property of function parameter 'apiPermission'  no-param-reassign
  214:13  error  Assignment to property of function parameter 'apiPermission'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\base\DateAndPeriodBasedReport.ts
  39:7  error  Assignment to property of function parameter 'option'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\base\SpoCommand.ts
  75:7  error  Assignment to property of function parameter 'options'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\file\commands\convert\convert-pdf.ts
  306:59  error  Assignment to function parameter 'currentChunk'  no-param-reassign
  322:54  error  Assignment to function parameter 'currentChunk'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\file\commands\file-add.ts
  217:59  error  Assignment to function parameter 'currentChunk'  no-param-reassign
  243:56  error  Assignment to function parameter 'currentChunk'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\file\commands\file-list.ts
  227:32  error  Assignment to property of function parameter 'file'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\flow\commands\environment\environment-list.ts
  37:11  error  Assignment to property of function parameter 'e'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\flow\commands\flow-list.ts
  63:11  error  Assignment to property of function parameter 'i'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\flow\commands\run\run-list.ts
  58:11  error  Assignment to property of function parameter 'i'  no-param-reassign
  59:11  error  Assignment to property of function parameter 'i'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\graph\commands\changelog\changelog-list.ts
  173:5  error  Assignment to property of function parameter 'changelog'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\pa\commands\app\app-get.ts
  146:5  error  Assignment to property of function parameter 'app'  no-param-reassign
  147:5  error  Assignment to property of function parameter 'app'  no-param-reassign
  148:5  error  Assignment to property of function parameter 'app'  no-param-reassign
  149:5  error  Assignment to property of function parameter 'app'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\pa\commands\app\app-list.ts
  82:11  error  Assignment to property of function parameter 'a'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\pa\commands\connector\connector-list.ts
  57:11  error  Assignment to property of function parameter 'c'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\pa\commands\environment\environment-list.ts
  37:11  error  Assignment to property of function parameter 'e'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\pp\commands\environment\environment-list.ts
  76:11  error  Assignment to property of function parameter 'e'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\spfx\commands\project\JsonRule.ts
  44:7  error  Assignment to property of function parameter 'jsonFile'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\spfx\commands\project\project-doctor.ts
  101:9  error  Assignment to property of function parameter 'o'  no-param-reassign
  212:9  error  Assignment to property of function parameter 'f'  no-param-reassign
  216:9  error  Assignment to property of function parameter 'f'  no-param-reassign
  220:9  error  Assignment to property of function parameter 'f'  no-param-reassign
  224:9  error  Assignment to property of function parameter 'f'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\spfx\commands\project\project-externalize.ts
  70:9  error  Assignment to property of function parameter 'o'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\spfx\commands\project\project-upgrade.ts
  136:9  error  Assignment to property of function parameter 'o'  no-param-reassign
  289:9  error  Assignment to property of function parameter 'f'  no-param-reassign
  293:9  error  Assignment to property of function parameter 'f'  no-param-reassign
  297:9  error  Assignment to property of function parameter 'f'  no-param-reassign
  301:9  error  Assignment to property of function parameter 'f'  no-param-reassign
  307:9  error  Assignment to property of function parameter 'f'  no-param-reassign
  308:9  error  Assignment to property of function parameter 'f'  no-param-reassign
  313:9  error  Assignment to property of function parameter 'f'  no-param-reassign
  314:9  error  Assignment to property of function parameter 'f'  no-param-reassign
  315:9  error  Assignment to property of function parameter 'f'  no-param-reassign
  316:9  error  Assignment to property of function parameter 'f'  no-param-reassign
  329:9  error  Assignment to property of function parameter 'f'  no-param-reassign
  330:9  error  Assignment to property of function parameter 'f'  no-param-reassign
  331:9  error  Assignment to property of function parameter 'f'  no-param-reassign
  332:9  error  Assignment to property of function parameter 'f'  no-param-reassign
  333:9  error  Assignment to property of function parameter 'f'  no-param-reassign
  334:9  error  Assignment to property of function parameter 'f'  no-param-reassign
  335:9  error  Assignment to property of function parameter 'f'  no-param-reassign
  340:9  error  Assignment to property of function parameter 'f'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\spfx\commands\spfx-doctor.ts
  524:7  error  Assignment to property of function parameter 'args'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\spo\commands\app\app-teamspackage-download.ts
  171:7   error  Assignment to property of function parameter 'appInfo'  no-param-reassign
  203:11  error  Assignment to property of function parameter 'appInfo'  no-param-reassign
  205:13  error  Assignment to property of function parameter 'appInfo'  no-param-reassign
  212:13  error  Assignment to property of function parameter 'appInfo'  no-param-reassign
  219:9   error  Assignment to property of function parameter 'appInfo'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\spo\commands\contenttype\contenttype-set.ts
  195:9  error  Assignment to property of function parameter 'object'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\spo\commands\customaction\customaction-add.ts
  234:9  error  Assignment to property of function parameter 'args'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\spo\commands\customaction\customaction-clear.ts
  131:7   error  Assignment to property of function parameter 'options'  no-param-reassign
  136:11  error  Assignment to property of function parameter 'options'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\spo\commands\customaction\customaction-get.ts
  182:7   error  Assignment to property of function parameter 'options'  no-param-reassign
  191:11  error  Assignment to property of function parameter 'options'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\spo\commands\customaction\customaction-list.ts
  104:38  error  Assignment to property of function parameter 'a'        no-param-reassign
  142:7   error  Assignment to property of function parameter 'options'  no-param-reassign
  150:11  error  Assignment to property of function parameter 'options'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\spo\commands\customaction\customaction-remove.ts
  191:7   error  Assignment to property of function parameter 'options'  no-param-reassign
  200:11  error  Assignment to property of function parameter 'options'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\spo\commands\customaction\customaction-set.ts
  218:9   error  Assignment to property of function parameter 'args'     no-param-reassign
  261:7   error  Assignment to property of function parameter 'options'  no-param-reassign
  270:11  error  Assignment to property of function parameter 'options'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\spo\commands\externaluser\externaluser-list.ts
  155:20  error  Assignment to property of function parameter 'e'  no-param-reassign
  163:13  error  Assignment to property of function parameter 'e'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\spo\commands\field\field-add.ts
   97:13  error  Assignment to function parameter 'o'  no-param-reassign
  157:7   error  Assignment to function parameter 'o'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\spo\commands\file\file-add.ts
  444:7   error  Assignment to property of function parameter 'info'  no-param-reassign
  476:17  error  Assignment to property of function parameter 'info'  no-param-reassign
  480:13  error  Assignment to property of function parameter 'info'  no-param-reassign
  497:13  error  Assignment to property of function parameter 'info'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\spo\commands\file\file-version-get.spec.ts
  153:11  error  Assignment to property of function parameter 'opts'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\spo\commands\folder\folder-roleassignment-remove.ts
  116:11  error  Assignment to property of function parameter 'args'  no-param-reassign
  120:11  error  Assignment to property of function parameter 'args'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\spo\commands\hubsite\hubsite-list.ts
  108:13  error  Assignment to property of function parameter 'h'           no-param-reassign
  130:11  error  Assignment to function parameter 'batchNumber'             no-param-reassign
  139:13  error  Assignment to property of function parameter 'reqOptions'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\spo\commands\list\list-get.ts
  124:11  error  Assignment to property of function parameter 'r'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\spo\commands\list\list-list.ts
  68:9  error  Assignment to property of function parameter 'l'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\spo\commands\list\list-roleassignment-add.ts
  152:7  error  Assignment to property of function parameter 'args'  no-param-reassign
  154:9  error  Assignment to property of function parameter 'args'  no-param-reassign
  158:9  error  Assignment to property of function parameter 'args'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\spo\commands\list\list-roleassignment-remove.ts
  141:11  error  Assignment to property of function parameter 'args'  no-param-reassign
  145:11  error  Assignment to property of function parameter 'args'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\spo\commands\list\list-webhook-list.ts
  126:11  error  Assignment to property of function parameter 'w'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\spo\commands\listitem\listitem-list.ts
  231:51  error  Assignment to property of function parameter 'v'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\spo\commands\listitem\listitem-roleassignment-remove.ts
  151:11  error  Assignment to property of function parameter 'args'  no-param-reassign
  155:11  error  Assignment to property of function parameter 'args'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\spo\commands\page\Page.ts
  87:12  error  Assignment to property of function parameter 'control'  no-param-reassign
  90:8   error  Assignment to property of function parameter 'control'  no-param-reassign
  94:14  error  Assignment to property of function parameter 'control'  no-param-reassign
  98:14  error  Assignment to property of function parameter 'control'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\spo\commands\page\page-clientsidewebpart-add.ts
  327:11  error  Assignment to property of function parameter 'w'        no-param-reassign
  445:10  error  Assignment to property of function parameter 'webPart'  no-param-reassign
  459:8   error  Assignment to property of function parameter 'webPart'  no-param-reassign
  460:7   error  Assignment to property of function parameter 'webPart'  no-param-reassign
  474:9   error  Assignment to property of function parameter 't'        no-param-reassign

D:\workspace\cli-microsoft365\src\m365\spo\commands\page\page-copy.ts
  154:7  error  Assignment to function parameter 'value'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\spo\commands\propertybag\propertybag-base.ts
  119:7   error  Assignment to function parameter 'objKey'    no-param-reassign
  120:7   error  Assignment to function parameter 'objValue'  no-param-reassign
  130:9   error  Assignment to function parameter 'objValue'  no-param-reassign
  136:11  error  Assignment to function parameter 'objValue'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\spo\commands\roledefinition\roledefinition-list.ts
  81:7  error  Assignment to property of function parameter 'r'  no-param-reassign
  82:7  error  Assignment to property of function parameter 'r'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\spo\commands\serviceprincipal\serviceprincipal-grant-list.ts
  50:18  error  Assignment to property of function parameter 'r'  no-param-reassign
  51:18  error  Assignment to property of function parameter 'r'  no-param-reassign
  52:18  error  Assignment to property of function parameter 'r'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\spo\commands\site\site-rename.ts
  164:5  error  Assignment to function parameter 'iteration'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\spo\commands\sitedesign\sitedesign-rights-list.ts
  71:9  error  Assignment to property of function parameter 'p'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\spo\commands\sitedesign\sitedesign-run-list.ts
  97:11  error  Assignment to property of function parameter 'd'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\spo\commands\tenant\tenant-recyclebinitem-list.ts
  47:11  error  Assignment to property of function parameter 's'  no-param-reassign
  48:11  error  Assignment to property of function parameter 's'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\spo\commands\term\term-group-list.ts
  48:11  error  Assignment to property of function parameter 't'  no-param-reassign
  49:11  error  Assignment to property of function parameter 't'  no-param-reassign
  50:11  error  Assignment to property of function parameter 't'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\spo\commands\term\term-list.ts
  130:11  error  Assignment to property of function parameter 't'  no-param-reassign
  131:11  error  Assignment to property of function parameter 't'  no-param-reassign
  132:11  error  Assignment to property of function parameter 't'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\spo\commands\term\term-set-list.ts
  109:11  error  Assignment to property of function parameter 't'  no-param-reassign
  110:11  error  Assignment to property of function parameter 't'  no-param-reassign
  111:11  error  Assignment to property of function parameter 't'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\spo\commands\web\web-roleassignment-add.ts
  118:7  error  Assignment to property of function parameter 'args'  no-param-reassign
  121:9  error  Assignment to property of function parameter 'args'  no-param-reassign
  125:9  error  Assignment to property of function parameter 'args'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\spo\commands\web\web-roleassignment-remove.ts
  107:11  error  Assignment to property of function parameter 'args'  no-param-reassign
  111:11  error  Assignment to property of function parameter 'args'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\teams\commands\app\app-list.ts
  140:11  error  Assignment to property of function parameter 't'  no-param-reassign
  141:11  error  Assignment to property of function parameter 't'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\teams\commands\chat\chat-message-list.ts
  69:11  error  Assignment to property of function parameter 'i'  no-param-reassign
  85:11  error  Assignment to property of function parameter 'i'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\teams\commands\message\message-list.ts
  94:11  error  Assignment to property of function parameter 'i'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\teams\commands\message\message-reply-list.ts
  76:11  error  Assignment to property of function parameter 'i'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\teams\commands\tab\tab-list.ts
  72:10  error  Assignment to property of function parameter 'i'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\teams\commands\team\team-add.ts
  187:7  error  Assignment to function parameter 'dots'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\teams\commands\user\user-app-list.ts
  87:10  error  Assignment to property of function parameter 'i'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\yammer\commands\group\group-list.ts
  108:46  error  Assignment to function parameter 'page'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\yammer\commands\message\message-list.ts
  130:11  error  Assignment to property of function parameter 'args'  no-param-reassign
  234:9   error  Assignment to property of function parameter 'm'     no-param-reassign

D:\workspace\cli-microsoft365\src\m365\yammer\commands\user\user-list.ts
  157:48  error  Assignment to function parameter 'page'  no-param-reassign

D:\workspace\cli-microsoft365\src\m365\yammer\commands\yammer-search.ts
  241:44  error  Assignment to function parameter 'page'  no-param-reassign

D:\workspace\cli-microsoft365\src\request.spec.ts
  383:7  error  Assignment to property of function parameter 'options'  no-param-reassign
  589:7  error  Assignment to property of function parameter 'options'  no-param-reassign

D:\workspace\cli-microsoft365\src\request.ts
   93:9   error  Assignment to property of function parameter 'config'   no-param-reassign
  110:12  error  Assignment to property of function parameter 'error'    no-param-reassign
  119:5   error  Assignment to property of function parameter 'options'  no-param-reassign
  124:5   error  Assignment to property of function parameter 'options'  no-param-reassign
  129:5   error  Assignment to property of function parameter 'options'  no-param-reassign
  134:5   error  Assignment to property of function parameter 'options'  no-param-reassign
  139:5   error  Assignment to property of function parameter 'options'  no-param-reassign
  144:5   error  Assignment to property of function parameter 'options'  no-param-reassign
  167:22  error  Assignment to property of function parameter 'options'  no-param-reassign
  170:22  error  Assignment to property of function parameter 'options'  no-param-reassign
  173:15  error  Assignment to property of function parameter 'options'  no-param-reassign

D:\workspace\cli-microsoft365\src\utils\formatting.ts
  40:9  error  Assignment to property of function parameter 'filtered'  no-param-reassign

D:\workspace\cli-microsoft365\src\utils\fsUtil.ts
  78:7  error  Assignment to function parameter 'dest'  no-param-reassign

D:\workspace\cli-microsoft365\src\utils\md.ts
  92:5  error  Assignment to function parameter 'md'  no-param-reassign
  94:7  error  Assignment to function parameter 'md'  no-param-reassign

D:\workspace\cli-microsoft365\src\utils\sinonUtil.ts
  8:7  error  Assignment to function parameter 'method'  no-param-reassign

D:\workspace\cli-microsoft365\src\utils\spo.ts
   93:11  error  Assignment to function parameter 'context'         no-param-reassign
  111:9   error  Assignment to function parameter 'currentContext'  no-param-reassign
  118:11  error  Assignment to function parameter 'dots'            no-param-reassign
  179:7   error  Assignment to function parameter 'dots'            no-param-reassign
  345:7   error  Assignment to function parameter 'webFullUrl'      no-param-reassign
  348:5   error  Assignment to function parameter 'folderToEnsure'  no-param-reassign

D:\workspace\cli-microsoft365\src\utils\urlUtil.ts
   49:7  error  Assignment to function parameter 'folderRelativePath'  no-param-reassign
   55:9  error  Assignment to function parameter 'folderRelativePath'  no-param-reassign
   62:9  error  Assignment to function parameter 'folderRelativePath'  no-param-reassign
  174:7  error  Assignment to function parameter 'serverRelativeUrl'   no-param-reassign
  187:7  error  Assignment to function parameter 'baseUrl'             no-param-reassign
  192:7  error  Assignment to function parameter 'relativeUrl'         no-param-reassign
  197:7  error  Assignment to function parameter 'relativeUrl'         no-param-reassign

martinlingstuyl commented 1 year ago

That's quite a list and a lot of work probably. But I do think this is they way to go. A good rule like this will eventually improve the quality of our code...

Adam-it commented 1 year ago

That's quite a list and a lot of work probably. But I do think this is they way to go. A good rule like this will eventually improve the quality of our code...

I agree. Since this is a lot of work I would not push it for v6 😉. Also not to break the current PR's which maybe we could still merge before v6 release.

waldekmastykarz commented 1 year ago

Also, many internal private methods inputs are overwritten like when setting webpart properties etc. this will need refactoring as well.

Sometimes this is by design. If the methods args are not coming from user input, and there's a valid need to overwrite them, we should be able to do so.

TBH I am also not sure we should push it before v6 release as it could make many current PR's not valid anymore 🤔

We'll always have open PRs that could be possibly be affected by this, so I don't think we'll be able to escape some breaking changes, v6 or not. That's just inherent for a refactoring of that sort.

Adam-it commented 1 year ago

ok so if we all agree I will start working on resolving those issues and if something will be 'by design' I will leave it as is so that we may double-check during PR review

Adam-it commented 9 months ago

I have been starting and then leaving this activity undone several times no and it seems there is always something with a higher priority that keeps me from finishing this one. Even now I have couple of other more important tasks on me before I find time on this issue so I guess it's only fair I stop blocking this one and make room for someone which will be a better fit than me to get the job done 😅. Sorry for the hold up and let's make this open for others again. I will still track this issue on my plan list and if in future I will find time and this will be still open I will take another go on this

pnp / cli-microsoft365

Small code refactor - avoid overwriting args and consider them as read only #3772

🤔 Problem

Commands