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

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

Merged
merged 3 commits into from Jun 4, 2018

Conversation

bcardiff
Copy link
Member

@bcardiff 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 June 4, 2018 10:44
@bcardiff bcardiff changed the title Add File.same?(path) with follow_symlinks option 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in the next commit

Copy link
Contributor

@ysbaddaden ysbaddaden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Allright!

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

@bcardiff
Copy link
Member Author

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.

Copy link
Contributor

@RX14 RX14 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@bcardiff
Copy link
Member Author

bcardiff commented Jun 4, 2018

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

@bcardiff bcardiff merged commit 4e48495 into crystal-lang:master Jun 4, 2018
@bcardiff bcardiff added this to the 0.25.0 milestone Jun 4, 2018
chris-huxtable pushed a commit to chris-huxtable/crystal that referenced this pull request Jun 6, 2018
@bcardiff bcardiff deleted the fix/file-info-equality branch June 26, 2018 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants