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

Allow indexing FileTree with string keys with path components in them #67

Open
mbauman opened this issue Nov 29, 2023 · 1 comment
Open

Comments

@mbauman
Copy link
Member

mbauman commented Nov 29, 2023

Currently:

julia> filetree["path/to/file.ext"]
ERROR: Path components cannot contain '/' or '\' (got "path/to/file.ext")
Stacktrace:
 [1] error(s::String)
   @ Base ./error.jl:35
 [2] joinpath(path::DataSets.RelPath, xs::String)
   @ DataSets ~/data/.julia/packages/DataSets/DWg6S/src/paths.jl:28
 [3] getindex(tree::BlobTree{JuliaHubData.DataRepoCache}, name::String)
   @ DataSets ~/data/.julia/packages/DataSets/DWg6S/src/BlobTree.jl:326
 [4] top-level scope
   @ REPL[96]:1

You need to use the special @path_str macro or explicitly construct a RelPath for this to work. It'd be nice to just allow using the complete key as a string.

@mbauman mbauman changed the title Allow indexing with string keys with path components in them Allow indexing FileTree with string keys with path components in them Nov 29, 2023
@mortenpi
Copy link
Member

mortenpi commented Dec 1, 2023

I am a little bit hesitant here, since it does look like it was a pretty conscious choice to not interpret slashes in Strings on the grounds that you may get weird platform-specific behavior. On unix-y systems, you might have a file with the name foo\bar, but we'd then interpret that as the path ("foo", "bar").

DataSets.jl/src/paths.jl

Lines 21 to 32 in c0e67b7

RelPath(::AbstractString) = error("RelPath(::String) is not defined to avoid ambiguities between operating systems. Use the `path\"...\"` string macro for path literals.")
RelPath(components::AbstractVector=String[]) = RelPath(convert(Vector{String}, components))
# Path manipulation.
function Base.joinpath(path::RelPath, xs::AbstractString...)
for x in xs
if '/' in x || '\\' in x
error("Path components cannot contain '/' or '\\' (got $(repr(x)))")
end
end
RelPath(vcat(path.components, xs...))
end

On the other hand, such file names are effectively prohibited in DataSets.jl datasets because of this restriction, so it would probably not break anything if we just apply the same logic that path"" uses on strings passed to getindex.

Implementation-wise, this would be a pretty trivial change.

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

2 participants