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

Add File.same?(path1, path2) with follow_symlinks option #6161

Merged
merged 3 commits into from Jun 4, 2018

Conversation

Projects
None yet
4 participants
@bcardiff
Member

bcardiff commented Jun 4, 2018

Follow up to #5584

Add File.same?(path1, path2) method to check if path refers to same file.
Rename the FileInfo#== to FileInfo#same_file? to have better semantics of comparing inodes.

@bcardiff bcardiff requested review from RX14 and ysbaddaden Jun 4, 2018

@bcardiff bcardiff force-pushed the bcardiff:fix/file-info-equality branch from 05b4431 to 4cf4ed8 Jun 4, 2018

@bcardiff bcardiff changed the title from Add File.same?(path) with follow_symlinks option to Add File.same?(path1, path2) with follow_symlinks option Jun 4, 2018

@@ -54,7 +54,7 @@ struct Crystal::System::FileInfo < ::File::Info
@stat.st_gid.to_u32
end
def ==(other : ::File::Info) : Bool
def same_file?(other : ::File::Info) : Bool

This comment has been minimized.

@RX14

RX14 Jun 4, 2018

Member

Can we add a File#== as well, which does info on both file instances and then same_file??

This comment has been minimized.

@bcardiff

bcardiff Jun 4, 2018

Member

I don't think the semantic of File#== to compare files is accurate. The file has more state that the file description.

This comment has been minimized.

@RX14

RX14 Jun 4, 2018

Member

It shouldn't do. But I guess this is fine for now...

@@ -54,7 +54,7 @@ struct Crystal::System::FileInfo < ::File::Info
@stat.st_gid.to_u32
end
def ==(other : ::File::Info) : Bool
def same_file?(other : ::File::Info) : Bool

This comment has been minimized.

@RX14

RX14 Jun 4, 2018

Member

This now needs an abstract def in src/file/info.cr.

This comment has been minimized.

@bcardiff

bcardiff Jun 4, 2018

Member

Updated in the next commit

@ysbaddaden

Allright!

I'm just not sure whether follow_symlinks should be true by default. I would tend to false myself.

@bcardiff

This comment has been minimized.

Member

bcardiff commented Jun 4, 2018

@ysbaddaden I agree, but the rest of the defaults for follow_symlink was true. I thought it was better to keep the same default.

@RX14

RX14 approved these changes Jun 4, 2018

I wonder the same about follow_symlinks, but it's not a huge deal so...

@bcardiff

This comment has been minimized.

Member

bcardiff commented Jun 4, 2018

Ok, I've changed the default to not follow symlinks.

@RX14

RX14 approved these changes Jun 4, 2018

@bcardiff bcardiff merged commit 4e48495 into crystal-lang:master Jun 4, 2018

4 checks passed

ci/circleci: test_darwin Your tests passed on CircleCI!
Details
ci/circleci: test_linux Your tests passed on CircleCI!
Details
ci/circleci: test_linux32 Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@bcardiff bcardiff added this to the 0.25.0 milestone Jun 4, 2018

chris-huxtable added a commit to chris-huxtable/crystal that referenced this pull request Jun 6, 2018

@bcardiff bcardiff deleted the bcardiff:fix/file-info-equality branch Jun 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment