Skip to content

Needed changes for Notebook UI Support #1321

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

Merged
merged 3 commits into from
Jul 14, 2020

Conversation

TylerLeonhardt
Copy link
Member

I wanted to make changes to PSES as small as possible because Notebook UI is a vscode feature, but some things had to be done.

  1. For CodeLens inside of a Notebook Cell, we need to ignore any results from the actual backing file because CodeLens looks at the file system. That would be counting duplicates.
  2. Had to add vscode-notebook-cell to our list of supporting schemes
  3. Made the Untitled path check more robust.

@TylerLeonhardt TylerLeonhardt requested a review from rjmholt as a code owner July 1, 2020 23:09
// For any vscode-notebook-cell, we need to ignore the backing file on disk.
if (scriptFile.DocumentUri.Scheme == "vscode-notebook-cell" &&
uri.Path == scriptFile.DocumentUri.Path &&
uri.Scheme == "file")
Copy link
Contributor

Choose a reason for hiding this comment

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

This check is probably simpler and should perhaps come second

DocumentUri uri = DocumentUri.From(foundReference.FilePath);
// For any vscode-notebook-cell, we need to ignore the backing file on disk.
if (scriptFile.DocumentUri.Scheme == "vscode-notebook-cell" &&
uri.Path == scriptFile.DocumentUri.Path &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to be case-insensitive?

Copy link
Member Author

Choose a reason for hiding this comment

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

They should be identical ... because in both cases it's using the actual file path on the file system.

@@ -217,8 +217,7 @@ internal static List<string> GetLinesInternal(string text)
internal static bool IsUntitledPath(string path)
{
Validate.IsNotNull(nameof(path), path);

return path.ToLower().StartsWith("untitled:");
return DocumentUri.From(path).Scheme.ToLower() != Uri.UriSchemeFile;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return DocumentUri.From(path).Scheme.ToLower() != Uri.UriSchemeFile;
return string.Equals(DocumentUri.From(path).Scheme, Uri.UriSchemeFile, StringComparison.OrdinalIgnoreCase);

@TylerLeonhardt
Copy link
Member Author

There is nothing really stopping this from going in now... but I'll wait until we're a bit further along with PowerShell/vscode-powershell#2789

@TylerLeonhardt TylerLeonhardt merged commit be84d4d into PowerShell:master Jul 14, 2020
@TylerLeonhardt TylerLeonhardt deleted the notebook-ui-support branch July 14, 2020 02:59
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.

2 participants