Fix table/plots not updating correctly on windows #3191
Conversation
| export const getRelativePattern = ( | ||
| path: string, | ||
| pattern: string | ||
| ): RelativePattern => { | ||
| for (const workspaceFolder of workspace.workspaceFolders || []) { | ||
| const workspaceFolderPath = workspaceFolder.uri.fsPath | ||
| if (isSameOrChild(workspaceFolderPath, path)) { | ||
| return new RelativePattern( | ||
| workspaceFolder, | ||
| join(relative(workspaceFolderPath, path), pattern) |
There was a problem hiding this comment.
Took wayyyy too long for me to realize this, but RelativePattern requires the pattern to be created with forward slashes 🤦. join will add backward slashes in a windows environment, thus breaking things in windows.
extension/src/fileSystem/watcher.ts
Outdated
| @@ -18,7 +19,7 @@ export const getRelativePattern = ( | |||
| if (isSameOrChild(workspaceFolderPath, path)) { | |||
| return new RelativePattern( | |||
| workspaceFolder, | |||
| join(relative(workspaceFolderPath, path), pattern) | |||
| joinWithForwardSlashes([relative(workspaceFolderPath, path), pattern]) | |||
There was a problem hiding this comment.
[Q] Can we create a new relativePattern file and move joinWithForwardSlashes and these function in there? Grouping the behaviour/functions together would give more clues about why it was done in the future.
[I] Would be good to get a regression test for the behaviour (maybe add to 'should watch the .git directory for updates' in extension/src/test/suite/experiments/data/index.test.ts).
There was a problem hiding this comment.
[Q] Can we create a new relativePattern file and move joinWithForwardSlashes and these function in there? Grouping the behaviour/functions together would give more clues about why it was done in the future.
Done!
[I] Would be good to get a regression test for the behaviour (maybe add to 'should watch the .git directory for updates' in extension/src/test/suite/experiments/data/index.test.ts).
Currently, we have a .git file watcher test set up for when the .git folder is outside of the workspace since that's how our demo project works. But I couldn't think of an uncomplicated way to test a workspace that has a local .git folder inside since we don't have a project like that in our repo... Is there something I'm misunderstanding/missing?
There was a problem hiding this comment.
not sure but can we create a set of nested subfolders inside of the demo project and then stub getGitRepositoryRoot to return that path or even stub getGitPath? Could then remove the top directory at the end of the test
There was a problem hiding this comment.
not sure but can we create a set of nested subfolders inside of the demo project and then stub getGitRepositoryRoot to return that path or even stub getGitPath? Could then remove the top directory at the end of the test
Will work on it in a followup!
|
[Q] Can you confirm if this also fixes #3102 |
julieg18
changed the title
Look like it does fix the issue! |
|
Code Climate has analyzed commit 70ca4db and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 100.0% (85% is the threshold). This pull request will bring the total coverage in the repository to 95.9% (0.0% change). View more on Code Climate. |
|
Going to go ahead and merge this! If we need more testing, I'll do so in a followup :) |
Demo
Extension.Development.Host.Experiments.-.example-get-started.-.Visual.Studio.Code.2023-01-31.13-27-43.mp4
Fixes #3103, fixes #3102