Fix an off-by-one error in getBaseDirectoriesFromRootDirs #55233
Conversation
typescript-bot
added
the
For Uncommitted Bug
|
You would need to add test in |
Isn't that for auto-import and not path completion? Or do these go through the same code path? |
|
I added a throw statement to |
|
@microsoft-github-policy-service agree company="Palantir" |
|
@sheetalkamat I added a fourslash test that failed before and succeeds now |
|
@sheetalkamat any chance this can make it to the 5.2 release? |
|
5.2 was branched off a while ago and main is now targeting 5.3; see #54298. Given this bug isn't a regression from 5.1, I wouldn't expect to see it backported unless it's causing major ecosystem pain. |
PhoebeSzmucer
force-pushed
the
ps/fix-55232
branch
from
1ff6da0 to
9abb627
Compare
|
|
||
| // Determine the path to the directory containing the script relative to the root directory it is contained within | ||
| const relativeDirectory = firstDefined(rootDirs, rootDirectory => containsPath(rootDirectory, scriptDirectory, basePath, ignoreCase) ? scriptDirectory.substr(rootDirectory.length) : undefined)!; // TODO: GH#18217 | ||
|
|
||
| // Now find a path for each potential directory that is to be merged with the one containing the script | ||
| return deduplicate<string>( | ||
| [...rootDirs.map(rootDirectory => combinePaths(rootDirectory, relativeDirectory)), scriptDirectory], | ||
| [...rootDirs.map(rootDirectory => combinePaths(rootDirectory, relativeDirectory)), scriptDirectory].map(baseDir => removeTrailingDirectorySeparator(baseDir)), |
There was a problem hiding this comment.
I had to also add removeTrailingDirectorySeparator here because otherwise some tests got broken due to duplicated entries with mismatched trailing slashes. It's probably a good idea to return consistent results anyway.
…55233) Co-authored-by: Phoebe Szmucer <[email protected]>
Fixes #55232
I couldn't find where this is tested (if it even is). If someone can help me out with that I can write a test.