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

Make play nice with FilePaths #20

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

oxinabox
Copy link

@oxinabox oxinabox commented Dec 31, 2018

Without this PR glob mostly works on FilePathsBase.jl types at least for local PosixPaths.
but it returns results that are Strings.

By allowing the type for the matches arrays to be determined based on the type of the inputs,
we can fix that.

This will break when/if rofinn/FilePathsBase.jl#15 comes though.
But til then at least this works.

cc @rofinn

@coveralls
Copy link

Coverage Status

Coverage decreased (-12.8%) to 79.851% when pulling 2164581 on oxinabox:ox/FilePathsFriendly into ed18d97 on vtjnash:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-12.8%) to 79.851% when pulling 2164581 on oxinabox:ox/FilePathsFriendly into ed18d97 on vtjnash:master.

@rofinn
Copy link

rofinn commented Dec 31, 2018

  1. Why would Drop AbstractString subtyping rofinn/FilePathsBase.jl#15 break this? Isn't matching the input and output types a good idea regardless of whether the filepaths need to define a specialized method?
  2. We could also include a specialized method in https://github.com/rofinn/FilePaths.jl along with the URI stuff.

@oxinabox
Copy link
Author

oxinabox commented Dec 31, 2018

Why would rofinn/FilePathsBase.jl#15 break this? Isn't matching the input and output types a good idea regardless of whether the filepaths need to define a specialized method?

It wouldn't break this PR,
but it would cause Glob.jl to stop working with FilePaths again (regardless of this PR),
since it has type-constraints on a bunch of path (prefix) arguments requiring them to be subtypes of AbstractString.
This PR does not relax all those type-constraints to Any.
(and I've not yet checked if that is possible.)

@oxinabox
Copy link
Author

@vtjnash bump

@vtjnash
Copy link
Owner

vtjnash commented Feb 16, 2022

Yes, I need to revisit this, now that rofinn/FilePathsBase.jl#15 is merged, and add tests for this. It has just been too low on my priority list for years.

I would like someday to write a package FakeFilesystem, which implements the FilePathsBase AbstractPath API, but only operates in-memory, and can be used as a test-only backend to test this package. Hopefully someday.

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.

4 participants