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

cleanup of Filesystem-related functions #12819

Merged
merged 10 commits into from
Nov 10, 2015
Merged

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented Aug 26, 2015

  1. groups Filesystem-related functions into a new module "Filesystem" (was "Base.FS")
  2. deprecates isreadable/writable/executable for filesystem paths, closing iswritable(::FileName) is useless and broken #7385
  3. adds a few missing isopen tests

@tkelman
Copy link
Contributor

tkelman commented Aug 26, 2015

capitalization bikeshed - Filesystem or FileSystem ?

@dpsanders
Copy link
Contributor

+1 for FileSystem

@vtjnash
Copy link
Member Author

vtjnash commented Aug 27, 2015

filesystem is a word

@StefanKarpinski
Copy link
Member

https://en.wikipedia.org/wiki/File_system: can be two words or one.

@StefanKarpinski
Copy link
Member

More prevalent usage seems to be two words; frequent abbreviation as "FS" seems to fit well with calling it FileSystem.

@stevengj
Copy link
Member

+1 for "Filesystem". Since the one-word version is already standard English, using CamelCase to concatenate the two-word version looks unnatural to me.

@ScottPJones
Copy link
Contributor

I don't care either way, just data points: Apache Hadoop API -> FileSystem, Java -> FileSystem.

@StefanKarpinski
Copy link
Member

@stevengj
Copy link
Member

There is Boost.Filesystem (although the actual namespace is boost::filesystem. On the other hand, there is the Microsoft Visual Basic FileSystem module, and LuaFileSystem, and QFileSystem. So it seems like FileSystem wins the popularity contest, but I don't have strong feelings either way.

@vtjnash
Copy link
Member Author

vtjnash commented Aug 27, 2015

Ruby seems pretty inconsistent there, since they use FileSystem, but Fs

@ScottPJones
Copy link
Contributor

Don't care, either way is better than just FS.

@StefanKarpinski
Copy link
Member

Since FileSystem seems most common and "file system" is a valid spelling so it's not incorrect, let's go with that.

@vtjnash
Copy link
Member Author

vtjnash commented Aug 27, 2015

that seems to be a lot of emphasis on "System", which I don't think is warranted given that I rarely see "file system" in a document without also finding "filesystem" in the same document somewhere else (indicating that this has rapidly become a word). I'm not really interested in bikeshedding this, but if someone wants to propose a shorter name, I might be listening. Python uses os.path. I would be OK going with Files.

@quinnj
Copy link
Member

quinnj commented Aug 27, 2015

What about Bash or Shell? I think it'd be cool to do Shell.cd and Shell.touch. Files isn't too bad either, but I think I'd lean towards FileSystem over Files, despite the brevity.

@vtjnash
Copy link
Member Author

vtjnash commented Aug 27, 2015

This doesn't use the shell or interact with it in any way. I still just don't like the way FileSystem seems to emphasize the word "System", since filesystem is already a valid regular word.

@tkelman
Copy link
Contributor

tkelman commented Aug 27, 2015

No need to rush this, I don't see why this would need to be an 0.4 target. iswritable is a more convenient name and easier to remember than fiddling with octal filemodes, and the user inaccuracy issue is a pretty confusing reason for deprecation.

@ScottPJones
Copy link
Contributor

A file system contains directories and files, and doesn't this API do more than just deal with files?
(which might imply name should be filesystem or FileSystem)
cd is changing process or shell state, not changing anything in a file system, so I wouldn't put it under FileSystem.

@StefanKarpinski
Copy link
Member

This is absolutely not going into 0.4.

@vtjnash
Copy link
Member Author

vtjnash commented Sep 12, 2015

rebased to be ready to merge when CI is happy

@tkelman
Copy link
Contributor

tkelman commented Sep 12, 2015

deprecation warnings should actually tell the user what replacement code to use.

@vtjnash
Copy link
Member Author

vtjnash commented Sep 13, 2015

deprecation warnings should actually tell the user what replacement code to use.

i can't really predict why the user thought they wanted to call these functions, so I pointed them to the man page where it describes why linux thinks of them as deprecated.

@tkelman
Copy link
Contributor

tkelman commented Sep 13, 2015

What are they documented to do? Deprecated functions should have something done about their docs, but not sure if that should happen at deprecation time or when the deprecation gets removed.

The deprecation warning is not the right place to send someone to read a man page because they called a function that you don't like. People are also using this function on systems that don't have such a man page. Just suggest the replacement code (st.mode & 0o222) > 0 in the warning.

I'm still not convinced there's a good reason to get rid of these since the replacement is inconvenient, low-level and unix-specific code. ok I think I get the issue now, the past behavior was not specific enough?

return (st.mode & 0o222) > 0
end
function isexecutable(st::FS.StatStruct)
depwarn("isexecutable is deprecated as it implied that the file would actually be executable by the user. see also the man page for `access`", :isreadable)
Copy link
Contributor

Choose a reason for hiding this comment

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

:isexecutable

@vtjnash
Copy link
Member Author

vtjnash commented Sep 13, 2015

What are they documented to do? ... ok I think I get the issue now, the past behavior was not specific enough?

what are they supposed to do? currently they are defined to return true if there might be a user (excluding root) that could have done the specified action at the instant prior to the time the question was answered. however, i can't think of a place case where that definition is useful.

the current docs say:

Returns `true` if the current user has permission to read `path`, `false` otherwise.

but that is not a question that stat can answer, as discussed in #7385

@tkelman
Copy link
Contributor

tkelman commented Sep 13, 2015

We don't usually give reasons for deprecation in warnings, we primarily tell the user what to change their code to. Here the ambiguity means users who care probably should do some nontrivial refactoring, but we should at least spell out in the warning how to accomplish what these functions used to do (even if technically buggy).

vtjnash added a commit that referenced this pull request Nov 10, 2015
@vtjnash vtjnash merged commit 232855e into master Nov 10, 2015
@vtjnash vtjnash deleted the jn/file_iswritable_cleanup branch November 10, 2015 18:09
yuyichao added a commit to JuliaLang/Compat.jl that referenced this pull request Nov 15, 2015
yuyichao added a commit to JuliaLang/Compat.jl that referenced this pull request Nov 15, 2015
yuyichao added a commit to JuliaInterop/ZMQ.jl that referenced this pull request Nov 15, 2015
yuyichao added a commit to JuliaInterop/ZMQ.jl that referenced this pull request Nov 16, 2015
yuyichao added a commit to JuliaLang/Compat.jl that referenced this pull request Nov 16, 2015
@dhoegh dhoegh mentioned this pull request Nov 16, 2015
fatteneder added a commit that referenced this pull request Feb 20, 2024
As discussed here:
#53286 (comment)

Readds the methods that were removed in
#12819.
KristofferC pushed a commit that referenced this pull request Feb 26, 2024
As discussed here:
#53286 (comment)

Readds the methods that were removed in
#12819.

(cherry picked from commit ea2b255)
tecosaur pushed a commit to tecosaur/julia that referenced this pull request Mar 4, 2024
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.