Adds support for interactive mode for disambiguation prompts. Closes #5053

[Adam-it]

Adds support for interactive mode for disambiguation prompts.

Linked Issue

Closes: #5053

Result

prompt.mp4

[martinlingstuyl]

Hi @Adam-it, what a great and huge job this is! You rock! 🎉 of course this can't go without a few comments, but I trust you already expected that 🤙

[Adam-it]

Hi @Adam-it, what a great and huge job this is! You rock! 🎉 of course this can't go without a few comments, but I trust you already expected that 🤙

Thanks @martinlingstuyl for the awesome review. I will amend those and add the missing 3 commands from the issue

[Adam-it]

@martinlingstuyl I:

• resolved your comments, thanks for the awesome feedback 👍
• added another command search externalconnection remove which was mentioned in the issue as 'problematic'
• merged with main and resolved conflicts

I didn't:

• add the site apppermission add and site apppermission set (also mentioned in the issue). TBH I am having some troubles with those and seem to always come to a point that I need to have nested Promise.Resolve which is rather hard to maintain 🤔. Instead of adding this functionality to the existing implementation I would rather suggest refactoring to async/await making it a looot easier, BUT since the refactor is not part of 50 other commands from this PR I would suggest opening a separate issue (I may already self-assigned myself and find a slot to implement it next month) in which we will refactor those two commands to async/await and add the disambiguation prompt as well. What say you? 😉

[Adam-it]

@martinlingstuyl I:

• resolved your comments, thanks for the awesome feedback 👍
• added another command search externalconnection remove which was mentioned in the issue as 'problematic'
• merged with main and resolved conflicts

I didn't:

• add the site apppermission add and site apppermission set (also mentioned in the issue). TBH I am having some troubles with those and seem to always come to a point that I need to have nested Promise.Resolve which is rather hard to maintain 🤔. Instead of adding this functionality to the existing implementation I would rather suggest refactoring to async/await making it a looot easier, BUT since the refactor is not part of 50 other commands from this PR I would suggest opening a separate issue (I may already self-assigned myself and find a slot to implement it next month) in which we will refactor those two commands to async/await and add the disambiguation prompt as well. What say you? 😉

sorry @martinlingstuyl for reposting but did you see my last response? is that acceptable for you?
is there anything else I may do (support you) in reviewing/rechecking this PR?

[martinlingstuyl]

Hi @Adam-it, it's been a while, but I'm back reviewing 😁 I'm having some additional feedback on the implementation here, I added some suggestions.

I'm interested to hear what you think of it!

[martinlingstuyl]

Oh, and also: could you fix the merge conflict as well? thanks in advance!

[Adam-it]

@martinlingstuyl ready for another round? 🙂

[martinlingstuyl]

Hi @Adam-it,
Ok so I went through all the files. There's quite some places where an array item is being updated.
That's a quick solution, but i do have code-smell feelings about that. 😄 I think we better just return the data at once.

What do you think? Do you Agree with me on this?

If you rebase on the latest main you'll also get my PR with listiteminstance ID deletions. They are in 8 files or so. Could you make sure those are executed above the code of the prompt?

There are quite some comments, but we're getting close now. 😉

[Adam-it]

Hi @Adam-it, Ok so I went through all the files. There's quite some places where an array item is being updated. That's a quick solution, but i do have code-smell feelings about that. 😄 I think we better just return the data at once.

What do you think? Do you Agree with me on this?

Sure. Not actually sure why I did the overwrites 🤔 but if it will work ok I will update the code

If you rebase on the latest main you'll also get my PR with listiteminstance ID deletions. They are in 8 files or so. Could you make sure those are executed above the code of the prompt?

👍

There are quite some comments, but we're getting close now. 😉

@martinlingstuyl did you also:

• rechecked the changes for every command locally if works as expected?
• take a look at the tests?

sorry for the additional questions but we aim to have it as part of v7 together with making interactive as default and I would like to avoid more turn arounds if possible 🙂

also @martinlingstuyl could you recheck your comments and add a bit more clarity to them 🙏.
Sorry for maybe some stupid comments 🙂 but my intention is to try to really understand the path you want me to take and then I am ready to 🌶️🚀 on this 👍

BTW thanks for another extensive review and sorry for a hard PR to handle 😉

[Adam-it]

@martinlingstuyl did you also:

• rechecked the changes for every command locally if works as expected?
• take a look at the tests?

sorry for the additional questions but we aim to have it as part of v7 together with making interactive as default and I would like to avoid more turn arounds if possible 🙂

also @martinlingstuyl could you recheck your comments and add a bit more clarity to them 🙏.
Sorry for maybe some stupid comments 🙂 but my intention is to try to really understand the path you want me to take and then I am ready to 🌶️🚀 on this 👍

BTW thanks for another extensive review and sorry for a hard PR to handle 😉

@martinlingstuyl I saw you answered to one comment but did you notice the other answer as well?

Also please double check the above so we may try to point out all required changes in a single (I hope last 😅) go instead of having a couple of turnarounds for a single comment which result every time in refactoring 100 files 😜🙂

[martinlingstuyl]

Also please double check the above so we may try to point out all required changes in a single (I hope last 😅) go instead of having a couple of turnarounds for a single comment which result every time in refactoring 100 files 😜🙂

Hi @Adam-it, I did a random spot check, so I did not execute all commands. Seeing the amount of commands and things I'd need to set up here to make that happen that seemed too much. But I did enough to be confident of your work.

I also checked the tests of course.

I understand you don't want too many turnarounds. The reason this happened is that the first few turnarounds were quite big redirections in my eyes, so I gave it back to you without checking everything, as too much was going to change as it was. I'm confident we'll make it in time though. 👍

[Adam-it]

Also please double check the above so we may try to point out all required changes in a single (I hope last 😅) go instead of having a couple of turnarounds for a single comment which result every time in refactoring 100 files 😜🙂

Hi @Adam-it, I did a random spot check, so I did not execute all commands. Seeing the amount of commands and things I'd need to set up here to make that happen that seemed too much. But I did enough to be confident of your work.

I also checked the tests of course.

I understand you don't want too many turnarounds. The reason this happened is that the first few turnarounds were quite big redirections in my eyes, so I gave it back to you without checking everything, as too much was going to change as it was. I'm confident we'll make it in time though. 👍

Ok good news and thanks for the confirmation. I started to get the impression that you come back with always a single find which in general is fine for me 🙂 but in this PR every change is a couple hour/100+ files refactor + rebase hell 😅. And it's hard for me to plan other stuff to do over the weekend if this returns multiple times instead of one or two 🙂.

Ok I will start working on those review comments. Please also double check my answers what was the intention: return object (hard part) or wrap around () and return .id (easy) 🙏

[martinlingstuyl]

Ok good news and thanks for the confirmation. I started to get the impression that you come back with always a single find which in general is fine for me 🙂 but in this PR every change is a couple hour/100+ files refactor + rebase hell 😅. And it's hard for me to plan other stuff to do over the weekend if this returns multiple times instead of one or two 🙂.

Hi @Adam-it, yes I get your point. 👍 We could have done one iteration less if I'd seen the message/errorMessage sooner. sorry for that 🙏

Ok I will start working on those review comments. Please also double check my answers what was the intention: return object (hard part) or wrap around () and return .id (easy) 🙏

I think I responded on one of the comments. to repeat:
"I personally dislike calling functions and using properties from the response in the same line. Less readable in my eyes.
But maybe this is a difference in tastes in code-styles. Feel free to ignore..."

Also: I made some mistakes in my suggestions (returning an entire object instead of just the property) you've already discovered that. Just fix those as you see fit.

Across the board there are two issues I'm commenting on:

• updating array items
• returning the property in the same line as the await.

Feel free to ignore the last.

[Adam-it]

Ok good news and thanks for the confirmation. I started to get the impression that you come back with always a single find which in general is fine for me 🙂 but in this PR every change is a couple hour/100+ files refactor + rebase hell 😅. And it's hard for me to plan other stuff to do over the weekend if this returns multiple times instead of one or two 🙂.

Hi @Adam-it, yes I get your point. 👍 We could have done one iteration less if I'd seen the message/errorMessage sooner. sorry for that 🙏

Ok I will start working on those review comments. Please also double check my answers what was the intention: return object (hard part) or wrap around () and return .id (easy) 🙏

I think I responded on one of the comments. to repeat:
"I personally dislike calling functions and using properties from the response in the same line. Less readable in my eyes.
But maybe this is a difference in tastes in code-styles. Feel free to ignore..."

Also: I made some mistakes in my suggestions (returning an entire object instead of just the property) you've already discovered that. Just fix those as you see fit.

Across the board there are two issues I'm commenting on:

• updating array items
• returning the property in the same line as the await.

Feel free to ignore the last.

Sir yes sir 🙂👍.
All clear now. Thanks 🙏👍

[Adam-it]

@martinlingstuyl ready for another go 👍
I took the approach you recommended 👍

[martinlingstuyl]

Check, I'll review it asap!

[martinlingstuyl]

Great work @Adam-it, will merge it soon.

[martinlingstuyl]

🎉 Merged manually, thank you!!