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

Clarify key to path edge cases with multiple delims #46

Open
aajtodd opened this issue Aug 23, 2024 · 0 comments
Open

Clarify key to path edge cases with multiple delims #46

aajtodd opened this issue Aug 23, 2024 · 0 comments

Comments

@aajtodd
Copy link
Contributor

aajtodd commented Aug 23, 2024

When implementing #45 several questions (example) came up around various edge cases w.r.t delimiters

I would expect the following behavior, you should probably add tests

key="many///delims-in-a-row" prefix="" delim="/" -> "many/delims-in-a-row"
key="//delims-at-start" prefix="" delim="/" -> "delims-at-start"
key="prefix///then-delims" prefix="prefix" delim="/" -> "then-delims"
key="prefix///then-delims" prefix="prefix/" delim="/" -> "then-delims"

NOTE: All but the first one here fail in the Java v2 TM today

what if stripped contains multiple delimiters in a row? e.g. key="a///1.txt" prefix="a/"
does path_clean handle that?

off the top of my head, if there are multiple delimiters, they should be collapsed to 1. Same way that typing
cd my-rust-project////////target gets the same result as typing cd my-rust-project/target


We need to clarify these edge cases and drive alignment and update the SEP if necessary. Many of them don't work the way we thought they should and actually result in errors in the current Java v2 TM. We don't want to create incompatibilities between TM implementations and make valid input for one TM suddenly invalid for another and vice versa.

There is also a test case in the SEP that is marked as being a success but actually fails currently in both Rust and Java TM:

            // FIXME - figure out if this test case is valid, Java v2 TM fails with exception stating it's outside the target directory
            // success_path_test("2023/Jan-1.png",	Some("2023"),	Some("-"),	"test/Jan/1.png"),
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

No branches or pull requests

1 participant