Skip to content

New activity output wrapper utils and execute step base#467

Merged
MicroFish91 merged 6 commits intomainfrom
mwf/activityWrapperUtils
Oct 8, 2023
Merged

New activity output wrapper utils and execute step base#467
MicroFish91 merged 6 commits intomainfrom
mwf/activityWrapperUtils

Conversation

@MicroFish91
Copy link
Copy Markdown
Contributor

@MicroFish91 MicroFish91 commented Sep 29, 2023

Summary:

  • New activity utils and activity try catch wrapper
  • New execute step base for automatically handling displaying of output logs/activityChildren corresponding to the execute step's success/fail state
  • Consolidate activity related util files into an activity folder
  • One example of implementation (LogAnalyticsCreateStep)

@MicroFish91 MicroFish91 marked this pull request as ready for review September 29, 2023 17:46
@MicroFish91 MicroFish91 requested a review from a team as a code owner September 29, 2023 17:46
*--------------------------------------------------------------------------------------------*/

import { AzExtTreeItem, AzureWizardExecuteStep, ExecuteActivityContext, IActionContext } from "@microsoft/vscode-azext-utils";
import { Progress } from "vscode";
Copy link
Copy Markdown
Member

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 do a type import for this (and a lot of these since it's all interfaces/abstract)

Copy link
Copy Markdown
Member

@nturinski nturinski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's nothing in here that really is a cause for blocking, so I'll just approve it. If you do make changes, I'll look at them again later

this.success = this.initSuccessOutput(context);
this.fail = this.initFailOutput(context);

await tryCatchActivityWrapper(() => this.executeCore(context, progress), context, this.success, this.fail, this.options);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there really any need to make tryCatchActivityWrapper a util function? I feel like it makes it a little bit more complicated than just having a try/catch clause in execute here. I don't think that you could really use this util function with anything other than this ExecuteActivityOutputStepBase.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point. I originally implemented it without the execute step base but forgot to extract this logic to sit together with it.

protected abstract executeCore(context: T, progress: Progress<{ message?: string | undefined; increment?: number | undefined }>): Promise<void>;
abstract shouldExecute(context: T): boolean;

protected abstract initSuccessOutput(context: T): ExecuteActivityOutput;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We talked about refactoring it, but I honestly don't have a very strong opinion about it. I think this does make it a bit more clear about the success and fail states. I think with my comment above, I'd just do the try/catch in the execute like this:

try {
  await this.executeCore(context, progress), context, this.options);
  output this.initSuccessOutput(context)
} catch (theseHands) {
  output this.initFailOutput(context)
}

I think after seeing this base step, that I agree that this is more clear. Though I don't really like the naming-- not sure what it is "initializing" really. I think createFailOutput or createFailChild maybe makes more sense to me?

public async execute(context: T, progress: Progress<{ message?: string | undefined; increment?: number | undefined }>): Promise<void> {
this.success = this.initSuccessOutput(context);
this.fail = this.initFailOutput(context);
this.success = this.createSuccessOutput(context);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, this feels much cleaner to me. Couple of things you can change:

  • You can do this.displayOutput at the end so you don't have to call it twice like this. I'd do something like this;
       let output;
       try {
            await this.executeCore(context, progress);
            output = this.createSuccessOutput(context);
        } catch (e) {
            output = this.createFailOutput(context);
            if (!this.options.shouldSwallowError) {
                throw e;
            } 
        } 
       
        this.displayOutput(context, output)    
  • You could also get rid of success and fail as properties on the interface. I personally think this is a bit cleaner. I don't think the finally clause will run

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's some rare cases where I might want to add onto the output log messages inside of executeCore, so I think it might be useful to have the success and fail outputs initialized and available ahead of time so they can be accessed within that method.

Copy link
Copy Markdown
Member

@nturinski nturinski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, I do prefer this and think it's cleaner.

@MicroFish91 MicroFish91 merged commit b4d8d0a into main Oct 8, 2023
@MicroFish91 MicroFish91 deleted the mwf/activityWrapperUtils branch October 8, 2023 04:50
@microsoft microsoft locked and limited conversation to collaborators Mar 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants