Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Task: refactor extensions file #5160

Draft
wants to merge 223 commits into
base: main
Choose a base branch
from
Draft

Conversation

thewahome
Copy link
Contributor

@thewahome thewahome commented Aug 14, 2024

Overview

Closes #5140

Notes

  • Introduces a base command class that brings in naming and executions. Now all commands can present their own name when being subscribed to and the execution of the command is named exactly the same
export abstract class Command {
  public abstract toString(): string;

  abstract execute(args: unknown): Promise<void> | void;
}
  • Extracts all command functions from the extension.ts file into individual commands that implement the base Command class

ElinorW and others added 30 commits April 3, 2024 17:47
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
@thewahome thewahome requested a review from a team as a code owner August 14, 2024 14:45
@thewahome thewahome marked this pull request as draft August 14, 2024 14:45
Copy link
Contributor

@petrhollayms petrhollayms left a comment

Choose a reason for hiding this comment

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

@thewahome Am I correctly assuming there we no changes in the logic?

async function updateStatusBarItem(context: ExtensionContext, kiotaStatusBarItem: StatusBarItem): Promise<void> {
try {
const version = await getKiotaVersion(context, kiotaOutputChannel);
if (!version) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we improve this, even in a separate issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No changes to the logic

if (!version) {
throw new Error("kiota not found");
}
kiotaStatusBarItem.text = `$(extensions-info-message) kiota ${version}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Kiota not kiota?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's from a refactor. The strings were carried as they were. I am not sure if changing them here would affect the localization downstream

Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

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

While I appreciate the much needed cleanup effort here, I'd like for us to hold merging this until the target PR gets merged to main.
A couple of reasons for that:

  • we're on tight deadlines to get the preview out.
  • additional functional changes need to happen in the main PR, and we don't want additional delays to deal with potential conflicts.
  • major refactoring right before releasing are a risky endeavour

Does that make sense?

@thewahome
Copy link
Contributor Author

@baywet
There's no problem. These changes ARE waiting for the base PR to get merged into the main PR

Copy link

sonarcloud bot commented Aug 15, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)
D Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

Base automatically changed from elinor/add-kiota-workspace to main August 22, 2024 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants