Implement Path type #5635

straight-shoota · 2018-01-24T13:34:24Z

This is a prototype of a Path type encapsulating filesystem paths to have a basis for further discussion in #5550

For now I have only refactored HTTP::StaticFileHandler to use the Path type to see how it can be used.

RX14 · 2018-01-24T14:10:25Z

spec/std/file_spec.cr

@@ -537,16 +537,16 @@ describe "File" do
    end

    it "expand_path for commoms unix path  give a full path" do
-      File.expand_path("/tmp/").should eq("/tmp")
+      File.expand_path("/tmp/").should eq("/tmp/")


Surely all the specs on file that deal with pure paths should be moved to path_spec. We needn't spec the Path wrapper and Path.

I wanted to leave it in until it gets completely replaced. This way it's easy to see that the behaviour is mostly the same except for a few details.

spec/std/path_spec.cr

src/path.cr

RX14 · 2018-01-24T15:08:58Z

src/path.cr

+    end
+  end
+
+  # Returns the root path component of this path or `nil` if it is not rooted.


What's the usecase for this?

I'm not entirely sure myself. But both Java and Python API's have this, so I'd guess it's probably useful for something... internally it's used by #anchor.

straight-shoota · 2018-01-24T22:06:45Z

Something is really strange with the compiler specs using Path...

The support/syntax helper includes Crystal into the top level namespace, so all over the specs Path is used to reference Crystal::Path. Now, the top level type ::Path overwrites that and of the specs no longer compile because types mismatch.

With 5ff295d I tried to replace Path with qualified Crystal::Path to use the correct type.
There are however some strange issues with some specs, for example spec/compiler/macro/macro_methods_spec.cr:1209:

assert_macro "x", %({{x.inputs}}), [ProcNotation.new([Crystal::Path.new("SomeType")] of ASTNode, Crystal::Path.new("SomeResult"))] of ASTNode, "[SomeType]"

This line produces a syntax error: expecting identifier 'class', not 'new'.

Previously it worked with Path.new. I can't really see what's the issue here... ist there any special syntax related to that example?

asterite · 2018-01-24T22:11:00Z

@straight-shoota

[] of X, Y -> Z

That's an array of procs.

Problem is, if you write

foo x, [] of X, Y

then compiler will think X, Y... refers to the type of the upcoming block, which never comes.

Solution: use parentheses:

foo x, ([] of X), Y

straight-shoota · 2018-01-24T23:31:25Z

But why is this an issue? The change just replaces Path with Crystal::Path...

Sija · 2018-01-24T23:48:07Z

I'd suggest changing name to Pathname as in Ruby since it clearly tells you the purpose of this Class, Path is IMO way too generic (hence collision with Crystal::Path).

RX14 · 2018-01-25T00:52:41Z

Pathname is just ugly... And its longer for what is hopefully a very well used class.

asterite · 2018-01-25T03:17:21Z

We just need not to include Crystal in our specs.

The language is ugly regarding types, there are no nice "imports" and no way to prevent types leaking all over the global namespace. We just have to accept that and move on.

bew · 2018-01-25T08:27:53Z

@asterite I think it's ok to include Crystal only for compiler specs, I don't think it's useful anywhere else.
For this we'd need to compile specs one category at a time (std, then compiler, then integration, etc..). I think this has also other advantages other than better controlling global namespace pollution, like less memory needed to build&run specs.

asterite · 2018-01-25T10:23:54Z

@bew problem is, Path will be required by prelude (it's used in File, which is required by prelude), so including Crystal will create a conflict with Path.

straight-shoota · 2018-01-25T10:29:26Z

Can't we just put the specs in Crystal namespace? That way Path would point to Crystal::Path (ASTNode) and Path (file system path) could by accessed as ::Path.

RX14 · 2018-01-25T11:10:56Z

I still don't understand why the parsing changes based on the addition of a qualifying name space.

You can have imports using private alias at the top level but you're always going to have a prefix.

I'm fine with putting the specs in a module.

asterite · 2018-01-25T11:56:10Z

Now I don't think the parsing changes or has anything to do with what I said above. I'll checkout this branch and investigate.

asterite · 2018-01-25T12:24:04Z

Found it. If you do:

foo([] of T, Y.new)

it parses "well" (Y.new) is a separate argument. The compiler will try to check if what comes after a comma can be a type, but because .new follows it's not.

However, with:

foo([] of T, X::Y.new)

the compiler fails to see this, because of the ::. We are just missing that rule in the parser.

RX14

I think this is pretty good on the API design side. I have lots of comments on the implementation but i'd rather @asterite, @ysbaddaden etc weighed in on the API first.

src/http/server/handlers/static_file_handler.cr

src/path.cr

src/http/server/handlers/static_file_handler.cr

RX14 · 2018-04-12T13:29:55Z

Would be good to get some more design opinions on this PR from the core team. I'm not fond of putting so much behaviour on the Type enum...

asterite · 2018-04-12T15:24:51Z

The more types we add to the std that's loaded by default (and Path will be one such type), the longer it takes to compile an empty file. So until we resolve that (and I'm more and more inclined to change the language for good) I can't comment on this. Well, my initial reaction is that I needed needed a Path type. I do need to split a path, or get the extension, or the base name, but all of that already exists as File class methods. Maybe they could be moved to a Path type, but meh.

RX14 · 2018-04-12T15:51:27Z

but all of that already exists as File class methods

The obvious response to that is that path operations tend to be composed quite often, and providing a path type makes composing these operations a lot easier because you don't have to specify File. every time.

I'm not particularly sympathetic to the compile time argument - I think that Path is one of the few things missing from the corelib, and after that is there really anything left to add?

Things like deque and set belong in the corelib less than path, but they're required by other parts of the corelib.

asterite · 2018-04-12T17:20:11Z

because you don't have to specify File. every time

Having to type less is never a good argument in my opinion (or at least I also thought like that, but not anymore)

I think that Path is one of the few things missing from the corelib, and after that is there really anything left to add?

A precise GC
Multithread support (whatever that is), with operations to simplify coding (work groups, sync operations, etc.)
printf is not yet good/complete in my opinion
I also always wanted to have scanf (I did a quick proof of concept in the past and it worked, though I think it won't work anymore with how I reimagine the language)
And possibly other things I might be forgetting

RX14 · 2018-04-12T17:47:25Z

Having to type less is never a good argument in my opinion

No, having to type less is never a good reason to sacrifice readability or understandability (i.e. adding too much magic), but here we do neither. The only thing we sacrifice for having to type less is adding a new class to the stdlib. Considering the current File primitives are demonstrably woefully inadequate on windows, and we'd have to basically rewrite them all anyway (see file_spec.cr in #5623), why not migrate to using Path at the same time.

A precise GC

will be written in C, rust (unlikely), not crystal. It's a GC'ed language itself - it's not suited for writing a GC.

Multithread support (whatever that is), with operations to simplify coding (work groups, sync operations, etc.)

fair enough, although I feel a lot of the higher-level concurrency abstractions can be in the stdlib not the corelib

printf is not yet good/complete in my opinion

That's more code, but not really a whole new class. If the code is only string ops and maths, then it will not be instantiated more than once, and will contribute much less to compile times.

I also always wanted to have scanf (I did a quick proof of concept in the past and it worked, though I think it won't work anymore with how I reimagine the language)

I don't think this is a good idea, writing lightweight parsers by hand is super easy in crystal, usually more robust, and generate better errors.

asterite · 2018-04-12T18:45:36Z

will be written in C, rust (unlikely), not crystal. It's a GC'ed language itself - it's not suited for writing a GC.

Go's GC is written in Go ;-)

RX14 · 2018-04-12T20:49:49Z

Go also doesn't have numerous places where hidden allocations occur. Even ones you wouldn't think about like .as.

straight-shoota · 2018-04-12T20:58:13Z

Can we please keep this thread clear of discussions about GC implementations and other Stdlib features? The gist is: this adds an additional type to corelib. We're also pretty sure that there will be no or only very few additions to that in the future. Having the class makes many things easier to write, but on the other hand adds an additional type to the inference algorithm for every program.
That's a trade-off to consider.

@RX14 I agree that having the behaviour defined on the enum is not great. It can easily be moved to private methods on Path, maybe with just a little bit more to type. Anyway, that's just an implementation detail.

RX14 · 2018-12-04T12:52:38Z

There should be no breaking changes here so this should be fine for 0.27.1

straight-shoota · 2018-12-04T13:04:00Z

There is technically a minor breaking change in File.expand_path which now preserves trailing slashes. If that's a hold back we could just revert the refactoring of that specific method - or all File methods for that matter. The focus should be on getting Path in the stdlib.

RX14 · 2018-12-15T16:42:08Z

@bcardiff this is still blocking a lot of Windows specs, i'd love it reviewed and merged in the next 2 weeks so I can resume working on Windows over Christmas.

sudo-nice · 2019-01-02T19:45:10Z

Any chance this PR will make it into 0.27.1?

straight-shoota · 2019-01-02T20:10:12Z

No, there are breaking changes to File class methods. It's already milestoned for 0.28.0 and needs a review from @bcardiff
I've rebased and squashed on current master.

sudo-nice · 2019-01-03T05:34:17Z

@straight-shoota Thank you.

src/path.cr

straight-shoota · 2019-03-03T17:45:59Z

Thanks, @r00ster91. Fixed typos and rebased on master.

This introduces a new Path type representing a filesystem path.

asterite · 2019-03-29T19:37:31Z

src/path.cr

+    ord = @name.compare(other.@name, case_insensitive: windows?)
+    return ord if ord != 0
+
+    @kind <=> other.@kind


I find this strange, but we can probably fix it later. Why a POSIX path is less than a WINDOWS path?

(and if that makes sense... shouldn't it be the other way around? 😛)

Also the PartialComparable is no longer needed due to latest changes in Comparable. But that can be changed later.

Hm, you are right! I can fix that in a PR after this one was merged (also removing PartialComparable).

~~removing~~ deprecating 🙈

Yes, though it isn't used anywhere else according to GitHub: https://github.com/search?q=language%3Acrystal+PartialComparable&type=Code

Regarding the original question: Two paths shouldn't be treated as equal if they are of different kinds, because that means different semantics. They shouldn't compare equal either, so this seems like a legitimate solution.
Could alternatively return nil of course when comparing different path types.

Returning nil sounds good to me

asterite

Let's do this! Thank you everyone for the incredible commitment and patience on this one! 🙇

straight-shoota · 2019-03-29T23:54:31Z

Yeah, can't believe i's been over a year... 🤣

RX14 requested changes Jan 24, 2018

View reviewed changes

straight-shoota force-pushed the jm/feature/path-type branch 3 times, most recently from 5297198 to 3a9e01b Compare January 24, 2018 18:34

straight-shoota force-pushed the jm/feature/path-type branch from 5ff295d to e860911 Compare January 25, 2018 11:36

straight-shoota force-pushed the jm/feature/path-type branch from e860911 to b6bfcab Compare January 25, 2018 12:06

RX14 requested changes Feb 10, 2018

View reviewed changes

src/http/server/handlers/static_file_handler.cr Outdated Show resolved Hide resolved

src/path.cr Show resolved Hide resolved

src/path.cr Outdated Show resolved Hide resolved

RX14 reviewed Feb 10, 2018

View reviewed changes

src/http/server/handlers/static_file_handler.cr Outdated Show resolved Hide resolved

straight-shoota mentioned this pull request Feb 10, 2018

Refactor implementation of array-like literals #5703

Open

straight-shoota mentioned this pull request Apr 3, 2018

File.join should handle "" separately #5905

Closed

RX14 removed this from the 0.28.0 milestone Dec 4, 2018

asterite mentioned this pull request Dec 7, 2018

Add Path type #5550

Closed

straight-shoota force-pushed the jm/feature/path-type branch from 58476bf to 43747c0 Compare January 2, 2019 20:09

straight-shoota added kind:feature topic:stdlib:files labels Jan 8, 2019

straight-shoota added this to the 0.28.0 milestone Feb 11, 2019

wooster0 suggested changes Mar 2, 2019

View reviewed changes

src/path.cr Outdated Show resolved Hide resolved

src/path.cr Outdated Show resolved Hide resolved

straight-shoota force-pushed the jm/feature/path-type branch from 43747c0 to 66c2ab3 Compare March 3, 2019 17:45

straight-shoota mentioned this pull request Mar 17, 2019

Fix formatting absolute paths #7560

Merged

straight-shoota added 2 commits March 25, 2019 12:37

Add Path type

0c31189

This introduces a new Path type representing a filesystem path.

Adapt File class methods to use Path implementation

3869d87

straight-shoota force-pushed the jm/feature/path-type branch from 66c2ab3 to 3869d87 Compare March 25, 2019 11:38

bcardiff approved these changes Mar 29, 2019

View reviewed changes

asterite reviewed Mar 29, 2019

View reviewed changes

asterite approved these changes Mar 29, 2019

View reviewed changes

bcardiff merged commit 653a2a7 into crystal-lang:master Mar 29, 2019

straight-shoota deleted the jm/feature/path-type branch March 29, 2019 23:54

straight-shoota mentioned this pull request May 12, 2019

Deprecate Path#dirname #7762

Closed

straight-shoota mentioned this pull request Dec 5, 2020

Std lib: File.absolute? File.relative? missing #5079

Closed

Implement Path type #5635

Implement Path type #5635

Conversation

straight-shoota commented Jan 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

straight-shoota commented Jan 24, 2018

asterite commented Jan 24, 2018

straight-shoota commented Jan 24, 2018

Sija commented Jan 24, 2018

RX14 commented Jan 25, 2018

asterite commented Jan 25, 2018

bew commented Jan 25, 2018

asterite commented Jan 25, 2018

straight-shoota commented Jan 25, 2018

RX14 commented Jan 25, 2018

asterite commented Jan 25, 2018

asterite commented Jan 25, 2018

RX14 left a comment

Choose a reason for hiding this comment

RX14 commented Apr 12, 2018

asterite commented Apr 12, 2018

RX14 commented Apr 12, 2018 • edited Loading

asterite commented Apr 12, 2018

RX14 commented Apr 12, 2018 • edited Loading

asterite commented Apr 12, 2018

RX14 commented Apr 12, 2018

straight-shoota commented Apr 12, 2018 • edited Loading

RX14 commented Dec 4, 2018

straight-shoota commented Dec 4, 2018

RX14 commented Dec 15, 2018

sudo-nice commented Jan 2, 2019

straight-shoota commented Jan 2, 2019

sudo-nice commented Jan 3, 2019

straight-shoota commented Mar 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asterite left a comment

Choose a reason for hiding this comment

straight-shoota commented Mar 29, 2019

straight-shoota commented Jan 24, 2018 •

edited

Loading

RX14 commented Apr 12, 2018 •

edited

Loading

RX14 commented Apr 12, 2018 •

edited

Loading

straight-shoota commented Apr 12, 2018 •

edited

Loading