Skip to content

test(stdune): Path.drop_prefix doesn't respect path boundaries#8870

Closed
Alizter wants to merge 1 commit intoocaml:mainfrom
Alizter:ps/branch/test_stdune___path_drop_prefix_doesn_t_respect_path_boundaries
Closed

test(stdune): Path.drop_prefix doesn't respect path boundaries#8870
Alizter wants to merge 1 commit intoocaml:mainfrom
Alizter:ps/branch/test_stdune___path_drop_prefix_doesn_t_respect_path_boundaries

Conversation

@Alizter
Copy link
Copy Markdown
Collaborator

@Alizter Alizter commented Oct 6, 2023

This is perhaps an unexpected result of using Path.drop_prefix. Perhaps another function Path.drop_path_prefix is needed instead if this behaviour is intentional.

This is perhaps an unexpected result of using `Path.drop_prefix`.
Perhaps another function `Path.drop_path_prefix` is needed instead if
this behaviour is intentional.

Signed-off-by: Ali Caglayan <alizter@gmail.com>
@Alizter Alizter mentioned this pull request Oct 6, 2023
@Alizter
Copy link
Copy Markdown
Collaborator Author

Alizter commented Oct 6, 2023

@anmonteiro since you implemented this function I am interested to hear what you think about this issue.

@anmonteiro
Copy link
Copy Markdown
Collaborator

This looks like a bug in drop_prefix instead. I think we want to drop the prefix if it's matched exactly as a path segment.

I think that wouldn't break anything existing usage, but I'm eager to see the results of that test run.

@Alizter Alizter added the bug label Oct 18, 2023
@Alizter Alizter closed this Oct 18, 2023
@Alizter Alizter deleted the ps/branch/test_stdune___path_drop_prefix_doesn_t_respect_path_boundaries branch October 18, 2023 09:07
@Alizter Alizter restored the ps/branch/test_stdune___path_drop_prefix_doesn_t_respect_path_boundaries branch October 18, 2023 09:08
@Alizter Alizter reopened this Oct 18, 2023
@Alizter
Copy link
Copy Markdown
Collaborator Author

Alizter commented Oct 18, 2023

Reopening as we don't have an issue. But the tests here have been subsumed by #8961.

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.

3 participants