Skip to content

Fix containsPath for paths that share a prefix#2509

Merged
robertbrignull merged 8 commits intomainfrom
robertbrignull/contains_path
Jun 12, 2023
Merged

Fix containsPath for paths that share a prefix#2509
robertbrignull merged 8 commits intomainfrom
robertbrignull/contains_path

Conversation

@robertbrignull
Copy link
Copy Markdown
Contributor

For context, in #2490 I started added my own method to determine if one paths contains another, but then I realised that we already have files.ts and containsPath.

Unfortunately containsPath uses a startsWith implementation and this isn't correct for examples like /foo and /foobar. In this case neither path is a parent of the other, but they do share /foo as a prefix.

This PR changes the implementation to use path.relative to do the work of deciding if a path contains another. We can also now remove the call to normalizePath from this method, as relative performs the same normalization as the resolve method. The only catch is we have to handle different drives on windows. See https://github.com/dsp-testing/robertbrignull-path-test/actions/runs/5244140271/jobs/9469730588 for some testing of this behaviour on linux vs windows.

I also made the methods a little clearer in my opinion, with more descriptive parameter names, documenting behaviour more closely, and removing the platform argument. We never actually pass a platform argument that isn't process.platform, and from my understanding it would be incorrect to ever do so because the behaviour of functions from path changes. It looks like we do in the tests, but actually the tests are skipped on mismatching platforms.

Changes are split up into separate commits to tell the renames from the new code and behaviour changes.

Checklist

  • CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
  • Issues have been created for any UI or other user-facing changes made by this pull request.
  • [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

Copy link
Copy Markdown
Member

@koesie10 koesie10 left a comment

Choose a reason for hiding this comment

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

Removing the platform argument makes sense to me, perhaps it made more sense when we didn't call into the path module to normalize paths.

Comment thread extensions/ql-vscode/src/pure/files.ts Outdated
Comment on lines +77 to +79
// On windows, if the two paths are in different drives, then the
// relative path will be an absolute path to the other drive.
!(process.platform === "win32" && isAbsolute(relativePath))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

On other platforms, receiving an absolute path back seems like it would also mean that parent does not contain child. What would prevent us from removing the check for process.platform here and simplifying this if-condition?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I wasn't very sure, but I think we could remove the check for process.platform and it wouldn't change behaviour.

I agree if we get back an absolute path then that's a bit weird and likely means that parent does not contain child. I can't think of a reason it would happen outside of windows, so I included the process.platform check as a safety to limit the extra behaviour to just that OS.

I'll take it out. Let me know what you think?

@robertbrignull robertbrignull enabled auto-merge June 12, 2023 14:40
@robertbrignull robertbrignull merged commit 400bde6 into main Jun 12, 2023
@robertbrignull robertbrignull deleted the robertbrignull/contains_path branch June 12, 2023 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants