Using util functions in favor of commands calling other commands to reuse code #4531
Comments
|
That are actually less commands than I expected! |
|
This reminds me of #3618 which we also have to create a bunch of issues for. |
We haven't gone to extremes to retrieving sites and other basic things through other commands which is why the impact is limited. Once we clean this up and take a look at the different API calls that we run, we'd probably find more candidates for centralization and removing duplicate code. |
|
It would be nice if we could get rid of commands calling other commands. Could I start working on some of these? |
Of course you can! Be sure when you create an issue for them to mention this epic, that way the issue will be linked. Also, to quickly create an issue here you can click on
|
|
@nicodecleyre: also, lets define the interface of the util and how it works before you go hacking away. That saves us time afterwards on discussion and possibly rework. So pick an issue, define how you want to solve it, and ask for opinions. Clear like that? |
Thx for the tip @Jwaegebaert! But that's something that seems to be only for an author of an issue or a maintainer.
Sure, let's take #5208 as an example then. For me the specification are:
Acceptance Criteria can be:
|
Ow pardon, had no clue that was a limited feature |
|
Very nice list of requirements @nicodecleyre! Let's include that in the actual issue descriptions. As for the utils, I have a more specific description in mind as well:
I think that would be the best way forward. |
|
Example for the app catalog: In Jsdoc setup. Something like follows: /**
* Gets app catalog from tenant
* returns undefined if none is configured
* writes to error stream if none is configured.
*/
public getTenantAppCatalog(logger: Logger):Promise<string | undefined>This is what we would need to discuss. |
|
Very good, I would like to suggest one more suggestion, the description of the parameters: /**
* Gets app catalog from tenant
* returns undefined if none is configured
* writes to error stream if none is configured.
* @param logger The logger object uses for the logging.
*/
public getTenantAppCatalog(logger: Logger):Promise<string | undefined>Regarding the issues, i'm not quite sure what you mean there but I would suggest that we focus on the command and refactor all the |
Exactly. Mine was just by example. |
That's okay as well. As long as we describe the necessary util(s) up front. |
Alright, here is an overview:
|
|
For the sake of completeness, the following 2 commands have already been refactored to util functions, so they may be removed from the list of this epic:
But following commands are missing from the list of this epic
I had some time this weekend and refactored the commands in this epic 😮💨. I will wait on submitting the PRs, waiting for your feedback on the above overview and adjust the refactoring where necessary before submitting. In addition, I will also triple check whether everything is still working properly before submitting the PRs 🙏. |
|
Hi @nicodecleyre, I like your enthousiasm. 😁 However: I do want to pull on the brakes for a minute. What I meant by describing the utils upfront was the following:
This would not need to be very long. but it gives us the possibility and time to decide/discuss the implementation details. It also means that you won't spend valuable time going down a path and having to refactor everything afterwards. Now that you already started it's maybe the easiest if you create issues for each command that you refactored. Take care to copy/write down the util interface and implementation details, so we can discuss it in the issue specs (instead of in the PR, which is harder for discussions) Is that okay for you? |
|
That's ok for me. Thank you @martinlingstuyl! |
|
I'd like to propose an alternative approach. Big PRs are hard to review and it costs a lot of time to get them right, leading to additional reviews, adjustments and merge conflicts. What if instead we break it into smaller steps and don't try to do everything at once. What I mean: say we've got a piece of code that would become a util and is used in 4 commands. Why not start with just one, extract the util, merge the PR and then refactor other commands one by one? It would simplify the review and isolate the work so that it doesn't depend on other things, and also is not affected by other changes that we've got in parallel. |
|
@waldekmastykarz if it's one util and 4 commands then we are more or less looking at a PR that will update 9 files (1 for the util, 4 files are the commands and 4 files are the tests for those commands). It does not seem that big. Also if we split this into smaller steps like:
and let's say only open and merge for PRs for step 1,2,3. Then let's say something happens, priority change and we are left with work for step 4 and 5 and also potentially we are left with some duplicated code. This is what we may have even now if we just refactor a single command and create util and not refactor the 'parent' commands. I agree that opening large PRs is hard to maintain but from what I suggested to group the current PRs into a bit bigger once we should not end up with PRs that update more than 20 files. At least that is what I expect. |
|
Our goal is to process PRs as quickly as possible. I'd avoid settling on arbitrary number of files like 10 or 20, because it doesn't express the amount of changes that are being submitted. The more changes PR authors submit at once, the longer it takes to review the PR and the bigger the odds for a conflict that the PR author will need to resolve before we can review the PR. Having more granular changes allows us to move faster, and we should work with our contributors so that they won't open too many PRs at once which again will take a long time to review and has a risk of conflicts. |
|
I'm sorry I've created this many PR's at once. But I don't really get it, what are we trying to discuss here? I thought everything was ok, we even had acceptance criteria and now we want to change things after the PR's are submitted? Merge conflicts will occur anyway, I've already put a lot of time in solving merge conflicts in every PR while the CLl moved to ESM. I hope not but it would be a shame if the PR's or better said the time I put into them is worthless. |
Thats true. But even though it was well prepared 👏 we noticed some things only after we started reviewing the PR's. This things happen and it is sometimes impossible to anticipate all things before the actual work (or part of work) is done.
For sure not. We understand it is a lot of effort and time spent for which we are truly grateful and also will do our very best so that it won't go to waste 👍
please double check my comment above
So it's important (....and this is something we only noticed just now, sorry for that) that we not only refactor the commands to use util instead of other commands (let's call them 'parent' commands) but also avoid code duplication. So if we created an util based on the 'parent' command logic we should also refactor the 'parent' command to use those utils so that we avoid the same code in two separate files (places). As I investigated this it will be not that easy as the created utils are quite large and specialized but it 'should' (famus last words) be possible 😉. if you agree I suggest also that we go step by step. So let's focus on PRs that introduced similar util (like spo page set and spo page add is a great example) lets for now only try to merge those two (leaving others in draft) so we will see if there is anything else we will find/learn along the way. Those PRs are in good shape, the only problem they have is the duplicated code like the one from |
|
Hi everyone! As #5301 has been merged, do you think we can open the next one? What would be the next steps here? |
hi @nicodecleyre, thanks for the reminder and the answer is yes, totally! |
Hi Adam! Thank you for your response. I would suggest that we stay in the |
|
@nicodecleyre what if we merge those 4 PRs into a single those create a common util method so it makes a lot of sense to be single PR and as I checked there is no duplication in the logic we have in the |
Sounds good to me 👍 will create a PR in the coming days |
Like we explained in #4486, we're going to replace the pattern of commands calling other commands to centralizing the shared code in util functions and using utils where needed.
Following commands call other commands and need to be refactored:
app getto use util instead of calling other command #5232app permission listto use util instead of calling other command #5234pa app getto use util instead of calling other command #5236planner roster member addto use util instead of calling other command #5238pp aibuildermodel removeto use util instead of calling other command #5240pp card cloneto use util instead of calling other command #5242pp card removeto use util instead of calling other command #5244pp chatbot removeto use util instead of calling other command #5246pp solution publishto use util instead of calling other command #5248pp solution publisher removeto use util instead of calling other command #5250pp solution removeto use util instead of calling other command #5252spfx project permissions grantto use util instead of calling other command #5254spo contenttype addto use util instead of calling other command #5256spo eventreceiver removeto use util instead of calling other command #5258spo file moveto use util instead of calling other command #5260spo file renameto use util instead of calling other command #5262spo file retentionlabel ensureto use util instead of calling other command #5264spo file retentionlabel removeto use util instead of calling other command #5266spo file roleassignment removeto use util instead of calling other command #5270spo file roleinheritance breakto use util instead of calling other command #5272spo file roleinheritance resetto use util instead of calling other command #5274spo file sharinglink clearto use util instead of calling other command #5276spo folder retentionlabel ensureto use util instead of calling other command #5278spo folder retentionlabel removeto use util instead of calling other command #5280spo folder roleassignment addto use util instead of calling other command #5282spo folder roleassignment removeto use util instead of calling other command #5284spo group member removeto use util instead of calling other command #5286spo listitem retentionlabel ensureto use util instead of calling other command #5292spo listitem roleassignment addto use util instead of calling other command #5294spo listitem roleassignment removeto use util instead of calling other command #5296spo page addto use util instead of calling other command #5298spo page setto use util instead of calling other command #5300spo serviceprincipal permissionrequest approveto use util instead of calling other command #5303spo site ensureto use util instead of calling other command #5305spo site setto use util instead of calling other command #5306spo tenant appcatalog addto use util instead of calling other command #5307spo tenant applicationcustomizer addto use util instead of calling other command #5308spo web roleassignment addto use util instead of calling other command #5310spo web roleassignment removeto use util instead of calling other command #5312teams meeting attendancereport listto use util instead of calling other command #5316teams meeting getto use util instead of calling other command #5318teams meeting listto use util instead of calling other command #5320teams team app listto use util instead of calling other command #5322viva connections app createto use util instead of calling other command #5324spo hubsite getto use util instead of calling other command #5290spo group setto use util instead of calling other command #5288spo fileto use util instead of calling other command #5268teams app installto use util instead of calling other command #5314spo tenant commandset addto use util instead of calling other command #5309The text was updated successfully, but these errors were encountered: