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

[wip] refactor AbsolutePath based on URL and RelativePath based on [String] #7215

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tomerd
Copy link
Member

@tomerd tomerd commented Dec 24, 2023

motivation: better support for Windows style paths

changes:

  • refactor AbsolutePath based on URL
  • refactor RelativePath based on [String]
  • remove redundant API surface area from RelativePath (anything not used outside tests)
  • remove RelativePath tests that covered redundant APIs

[TODO]

  • there are a number of tests in PathTests that are failing because the semantics is slightly different and the tests are coded to check for the existing semantics which may or may not be the right one. need to discuss which semantic we prefer and update the code or tests
  • this works for *nix style path, but should be adjusted to work for Windows. the fact we are using URL and [String] array should make that easier than the previous implementation and the genesis for this PR. cc @compnerd for help getting this work correctly on Windows.
  • improve the implementation to make it more efficient, address FIXME's etc

motivation: better support for Windows style paths

changes:
* refactor AbsolutePath based on URL
* refactor RelativePath based on [String]
* remove redundant API surface area from RelativePath (anything not used outside tests)
* remove RelativePath tests that covered redundant APIs
@compnerd
Copy link
Collaborator

From a quick look, we still have some API shape changes that need to be made. The concept of a root is not portable. The Windows model is a forest, not a tree. Furthermore, there are technically an infinite set of roots, so we cannot really enumerate them, but can check if a given path is a root of a tree.

@rauhul
Copy link
Contributor

rauhul commented Dec 26, 2023

From a quick look, we still have some API shape changes that need to be made. The concept of a root is not portable. The Windows model is a forest, not a tree. Furthermore, there are technically an infinite set of roots, so we cannot really enumerate them, but can check if a given path is a root of a tree.

Does system's FilePath type handle this?

@MaxDesiatov
Copy link
Member

MaxDesiatov commented Dec 29, 2023

Does system's FilePath type handle this?

AFAIU it unfortunately does not, and the lack of Windows CI on swift-system prevents us from checking that regressions in that area don't happen.

@tomerd
Copy link
Member Author

tomerd commented Jan 6, 2024

From a quick look, we still have some API shape changes that need to be made.

@compnerd agreed, and your help is needed to define how to make it so that it works better for Windows style paths

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.

None yet

4 participants