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

ignore / if the delimiter is something else #1850

Merged
merged 10 commits into from
Sep 12, 2023
Merged

ignore / if the delimiter is something else #1850

merged 10 commits into from
Sep 12, 2023

Conversation

Fil
Copy link
Contributor

@Fil Fil commented Sep 7, 2023

I don't think this is the correct fix! But at least it gives a unit test.

closes #1849

src/transforms/tree.js Outdated Show resolved Hide resolved
@mbostock
Copy link
Member

mbostock commented Sep 7, 2023

I added more test cases. Unfortunately I found there’s even a bug in the slash case!

@mbostock
Copy link
Member

mbostock commented Sep 7, 2023

Update: the test was slightly wrong, we need six (!) backslashes to have \/ in the node name. So the bug is in slashUnescape, which doesn’t unescape escaped backslashes, only escaped forward slashes.

@mbostock mbostock marked this pull request as ready for review September 12, 2023 15:05
@mbostock mbostock enabled auto-merge (squash) September 12, 2023 15:47
@Fil
Copy link
Contributor Author

Fil commented Sep 12, 2023

In the documentation, do we want to specify “a character" in

* **delimiter** - the path separator; defaults to forward slash (/)

@mbostock mbostock merged commit a063b22 into main Sep 12, 2023
1 check passed
@mbostock mbostock deleted the fil/tree-delimiter branch September 12, 2023 22:50
Fil added a commit that referenced this pull request Sep 13, 2023
* ignore / if the delimiter is something else

(note: I don't think this is the correct fix! But at least it gives a unit test)

closes #1849

* avoid string.replaceAll

* add delimiter test

* \/ needs three (six) backslashes!

* pass tests, but still not complete

* proper escape/unescape

* refactor logic

* lift delimiter code

* tweak error message

* refactor logic slightly

---------

Co-authored-by: Mike Bostock <[email protected]>
chaichontat pushed a commit to chaichontat/plot that referenced this pull request Jan 14, 2024
* ignore / if the delimiter is something else

(note: I don't think this is the correct fix! But at least it gives a unit test)

closes observablehq#1849

* avoid string.replaceAll

* add delimiter test

* \/ needs three (six) backslashes!

* pass tests, but still not complete

* proper escape/unescape

* refactor logic

* lift delimiter code

* tweak error message

* refactor logic slightly

---------

Co-authored-by: Mike Bostock <[email protected]>
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.

Unable to override default tree delimiter
2 participants