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

centralize vscode.Uri usage #539

Merged
merged 3 commits into from
Jun 12, 2024
Merged

centralize vscode.Uri usage #539

merged 3 commits into from
Jun 12, 2024

Conversation

pelikhan
Copy link
Member

@pelikhan pelikhan commented Jun 12, 2024

  • The vscode.Uri.parse function now includes a second argument, true, which allows parsing of URLs with strict mode enabled. This change affects multiple files including browser.ts, state.ts, and vshost.ts. 🌐
  • The applyEdits function in edit.ts now accepts an additional argument, state, which is an instance of ExtensionState. This change also affects the way URIs are created in the function. 📝
  • The activateFragmentCommands function in fragmentcommands.ts and other similar functions now use the host.toUri method to create URIs instead of vscode.Uri.file. This change is also reflected in multiple other files. 🔄
  • The ExtensionState class in state.ts has been significantly refactored. A large block of error handling code has been removed, and the saveScripts and applyEdits methods now use the host.toUri method. 🧹
  • The activateStatusBar function in statusbar.ts now uses the host property of state to create URIs. 📊
  • The activateTestController function in testcontroller.ts now uses the host.toUri method to create URIs. 🎮
  • A new method, toUri, has been added to the VSCodeHost class in vshost.ts. This method creates a vscode.Uri from a filename or URL. 🆕

In summary, these changes seem to centralize the creation of URIs in the VSCodeHost class, making the code more maintainable and consistent. 🎯

generated by pr-describe

Copy link
Member Author

@pelikhan pelikhan left a comment

Choose a reason for hiding this comment

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

/describe

Copy link
Member Author

@pelikhan pelikhan left a comment

Choose a reason for hiding this comment

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

/genai-describe

@pelikhan
Copy link
Member Author

/genai-describe

Copy link
Member Author

@pelikhan pelikhan left a comment

Choose a reason for hiding this comment

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

/genai-review

Copy link
Member Author

@pelikhan pelikhan left a comment

Choose a reason for hiding this comment

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

/genai-review

@pelikhan pelikhan marked this pull request as ready for review June 12, 2024 15:56
@pelikhan
Copy link
Member Author

/genai-review

Copy link

The changes in the GIT_DIFF show that the developer is refactoring the code to use a new method toUri from the host object to create URIs instead of using vscode.Uri.file. This is a good change as it centralizes the URI creation logic in one place, making the code more maintainable.

However, there is a potential issue with the toUri method. It checks if the input string is an absolute path or a URL, but it does not handle relative paths. If a relative path is passed to this method, it will be treated as an absolute path, which could lead to incorrect URIs.

Here is a suggested fix:

toUri(filenameOrUrl: string): vscode.Uri {
    const folder = this.projectUri;
    if (!filenameOrUrl) return folder;
    if (/^[a-z][a-z0-9+\-.]*:\/\//.test(filenameOrUrl))
        return vscode.Uri.parse(filenameOrUrl, true);
    if (this.path.isAbsolute(filenameOrUrl))
        return vscode.Uri.file(filenameOrUrl);
    else return vscode.Uri.file(path.join(folder.fsPath, filenameOrUrl));
}

This fix uses the path.join method to correctly handle relative paths.

Other than this issue, the changes look good. The developer has also removed some commented-out code, which is a good practice as it keeps the codebase clean.

So, with the above fix, LGTM 🚀

generated by pr-review

@pelikhan pelikhan merged commit 93aef5b into main Jun 12, 2024
6 checks passed
@pelikhan pelikhan deleted the commander-pr branch June 12, 2024 16:28
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.

1 participant