rofinn / FilePaths.jl

A type based approach to working with filesystem paths in julia
Other
81 stars 14 forks source link

Add integration with URIParser #22

Closed davidanthoff closed 6 years ago

davidanthoff commented 6 years ago

Fixes #18.

@rofinn, I completely agree with your general sentiment in #18, but am wondering whether we can merge this nevertheless for now.

Here the logic: I don't think it is realistic that this package might be made part of the stdlib for julia 1.0, that strikes me like a 1.1 project. I also feel that FilePaths is not stable enough at this point to ask URIParser to take a dependency on it. So I think as an intermediate step it makes most sense to have this functionality here in FilePaths.

Once FilePaths is ready to be moved into the stdlib (like, in a year or so), we can move the integration out of it and into URIParser.

Does that sound reasonable to you?

codecov-io commented 6 years ago

Codecov Report

:exclamation: No coverage uploaded for pull request base (master@38ad529). Click here to learn what that means. The diff coverage is 72.72%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #22   +/-   ##
=========================================
  Coverage          ?   72.72%           
=========================================
  Files             ?        1           
  Lines             ?       11           
  Branches          ?        0           
=========================================
  Hits              ?        8           
  Misses            ?        3           
  Partials          ?        0
Impacted Files Coverage Δ
src/uri.jl 72.72% <72.72%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 38ad529...2dbb655. Read the comment docs.

rofinn commented 6 years ago

How would you feel about having a FilePathsExt.jl package that could use Requires.jl to handle optional dependencies on packages like this (e.g., Glob.jl, URIParser.jl)?

davidanthoff commented 6 years ago

Yes, that would certainly work. Another option might be to rename this package here into FilePathsBase.jl, and then have FilePaths.jl include that stuff. I don't really care, but it would mean that things would just work if folks use FilePaths.jl. Happy to go with either option.

If we go with your idea, do you want to create the repo, or should I do it? I almost think we could actually skip Requires.jl for that one and just make FilePaths, URIParser and Glob a dependency? Neither of those is heavy weight, and it might just be easier.

rofinn commented 6 years ago

Okay, I'll create that package tomorrow. I'm fine with renaming this package to FilePathsBasse.jl, but I'm not sure how that renaming would work on METADATA? Would the following work?

  1. Rename repo from FilePaths.jl to FilePathsBase.jl
  2. Create FilePaths.jl repo and tag a release
  3. Create a single METADATA commit that both renames all existing FilePaths.jl files to FilePathsBase.jl and "registers" the new FilePaths.jl
davidanthoff commented 6 years ago

We should probably ask someone who knows how that works, but here is one way that probably works:

You create a new FilePathsBase.jl repo. You then just push the master branch from your local clone of FilePaths.jl to that new FilePathsBase.jl repo. Now both repos have the same commit as their master. Then you rename things in FilePathsBase.jl repo and register that package. In FilePaths.jl you add a commit that deletes everything except the integrations stuff.

The only thing in METADATA then would be registering FilePathsBase.jl as a new package, and then some new tags. I think that might be the least risky path?

rofinn commented 6 years ago

Okay, once FilePathsBase.jl is registered I'll update FilePaths.jl to just reexport those symbols and we can add URIParser and Glob back in. Would you be interested in supporting FileIO.jl?

davidanthoff commented 6 years ago

Hurray, that is excellent!

Yes, it would be nice if we could integrate with FileIO. At the same time I almost feel FileIO is ripe for a bit of a redesign. It is not entirely clear to me whether all the fancy "load packages behind the scene" stuff will still work on 0.7. But I might be wrong. I think the first thing I'll try is to use this in the VS Code extension where constantly have to juggle with file paths and URIs and it has to work on all platforms.

davidanthoff commented 6 years ago

I think this can also be merged now.

davidanthoff commented 6 years ago

Alright, I addressed both points you raised.

rofinn commented 6 years ago

Thanks, merging. I'm not sure what the deal with codecov is, but I've updated the README and if this happens on another PR then I'll dig into that more.

davidanthoff commented 6 years ago

Could you also tag a release?