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

RFC? Refactor stat #27917

Closed
wants to merge 18 commits into from
Closed

RFC? Refactor stat #27917

wants to merge 18 commits into from

Conversation

bramtayl
Copy link
Contributor

@bramtayl bramtayl commented Jul 3, 2018

When doing #25188, I noticed there was a lot of unused filesystem functions, and mostly stat ones. Though there didn't seem to be much interest in #25939, and its probably too late in the release cycle, I thought I'd put up a proposal anyway. This is totally incomplete, with only direct changes to the code. I thought I'd see what people think before going through the effort of cleaning up.

@bramtayl
Copy link
Contributor Author

bramtayl commented Jul 3, 2018

Summary:

  • lstat refactored into stat.
  • filemode, filesize, mtime, and ctime deprecated in favor of field accesses of stat.
  • ispath, isfifo, ischardev, isdir, isblockdev, isfile, islink, issocket, and issetuid refactored to filetype.
  • issetuid, issetgid, and issticky refactored to fileflags.
  • uperm, operm, and gperm refactored to permissions.

base/stat.jl Outdated
:read
else
error("Unknown permissions type")
end
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't handle multiple permission bits being set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, ok, I must have misunderstood the code. How would I fix it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought I would be ok with permissions returning a named tuple for all 3.

Copy link
Member

Choose a reason for hiding this comment

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

That part is ok, but each of user/group/other might have any combination of read/write/execute.

@StefanKarpinski
Copy link
Member

This is a pretty nice redesign. “Neither” is more commonly called “other”.

@@ -83,9 +83,13 @@ end
stat(fd::Integer) = stat(RawFD(fd))

"""
stat(file)
stat(file; link = false)
Copy link
Member

Choose a reason for hiding this comment

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

no spaces around = for keyword arguments

base/stat.jl Outdated
if link
stat(full_path)
else
lstat(full_path)
Copy link
Member

Choose a reason for hiding this comment

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

One too many spaces

@fredrikekre
Copy link
Member

I really really dislike the change from isdir(f) to filetype(f) == :dir. And ispath(f) to filetype(f) != :invalid is even worse.

@ararslan
Copy link
Member

ararslan commented Jul 5, 2018

Most of this doesn't really seem like an improvement to me, tbh.

@KristofferC
Copy link
Member

This PR is taking on too many things at once imo. The getfield on the StatStruct seems OK but some of the other things (like the filetype seems extremely verbose and brittle (what if you misspell the symbol slightly). It should use enums if anything.

@bramtayl
Copy link
Contributor Author

bramtayl commented Jul 6, 2018

I'd be ok with keeping a few convenience functions (ispath is definitely a keeper). I'm not sure I see the advantage of filetype returning enums rather than symbols but I could change that too.

@StefanKarpinski
Copy link
Member

Honestly, I think it's too late for this. We're well past feature freeze so any API that's not deeply broken is not up for change at this point. Maybe in Julia 2.0 or we could have an additional better API here and keep the old one, but that would likely go into a later 1.x release, not 1.0.

@bramtayl bramtayl closed this Jul 6, 2018
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.

6 participants