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

Added ismount function. #11279

Merged
merged 1 commit into from
May 27, 2015
Merged

Added ismount function. #11279

merged 1 commit into from
May 27, 2015

Conversation

malmaud
Copy link
Contributor

@malmaud malmaud commented May 15, 2015

Equivalent functionality of Python's os.path.ismount function:

os.path.ismount(path)
Return True if pathname path is a mount point: a point in a file system where a different file system has been mounted. The function checks whether path‘s parent, path/.., is on a different device than path, or whether path/.. and path point to the same i-node on the same device — this should detect mount points for all Unix and POSIX variants.

@tkelman
Copy link
Contributor

tkelman commented May 15, 2015

Seems harmless enough to have, non-exported. Could use a test though.

@jiahao
Copy link
Member

jiahao commented May 15, 2015

+1

@malmaud
Copy link
Contributor Author

malmaud commented May 15, 2015

OK, I'll add one.

@nalimilan
Copy link
Member

Should probably be exported (see exports.jl) and have some docs (see doc/stdlib/).

@tkelman
Copy link
Contributor

tkelman commented May 15, 2015

Docs wouldn't hurt, but is this widely used enough to export right away?

function ismount(path...)
path = abspath(joinpath(path...))
isdir(path) || return false
parent_path = splitdir(path)[1]
Copy link
Member

Choose a reason for hiding this comment

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

on linux, this is not necessarily the same as the parent directory.

@nalimilan
Copy link
Member

Docs wouldn't hurt, but is this widely used enough to export right away?

If we add it so that people can use it outside of Base, better assume it by exporting the function IMHO. Else this is just pushing people into relying on unofficial APIs. And since Python has it, it's probably worth having too (though it will probably have to move to a separate module when Base is refactored).

@tkelman
Copy link
Contributor

tkelman commented May 16, 2015

since Python has it, it's probably worth having too (though it will probably have to move to a separate module when Base is refactored).

Sounds good for now, on both counts. We should probably move a bunch of things to an FS module at some point. Doesn't need to be done now unless someone's feeling ambitious.

@malmaud
Copy link
Contributor Author

malmaud commented May 16, 2015

@vtjnash Good points. I'll look up the Python implementation and modify mine to be semantically identical.

@ivarne
Copy link
Member

ivarne commented May 16, 2015

I think it is a bad idea to add unexported functions to Base (unless they are an implementation detail of other functions).

I'm also negative to introducing a new function that we expect to move, so if we want a FS module, we should agree on the right name for the module and export it from there.

(sorry for closing, it was not intentional)

@ivarne ivarne closed this May 16, 2015
@ivarne ivarne reopened this May 16, 2015
@dpsanders
Copy link
Contributor

FS is not a good name for a module - we should respect the package naming rules. Better FileSystem

@tknopp
Copy link
Contributor

tknopp commented May 16, 2015

I am with @ivarne here. If things in Base are not important enough to be exported, then they should simply not be part of Base.

@tkelman
Copy link
Contributor

tkelman commented May 17, 2015

FS is not a good name for a module - we should respect the package naming rules. Better FileSystem

Good point. Base.FS happens to already exist, so FS would be the lazy choice. FileSystem would be much better.

I'm also negative to introducing a new function that we expect to move

There's a point there too, but I'm not sure if the onus should be on @malmaud who wants to add one function that has plenty of precedent in other languages, to undertake a major reorganization first. Anyone want to volunteer? I'll add it to my to-do list but might not get to it before 0.4.0.

@malmaud
Copy link
Contributor Author

malmaud commented May 25, 2015

Just for reference, this is the Python implementation:

def ismount(path):
    """Test whether a path is a mount point"""
    try:
        s1 = os.lstat(path)
    except OSError:
        # It doesn't exist -- so not a mount point. :-)
        return False
    else:
        # A symlink can never be a mount point
        if stat.S_ISLNK(s1.st_mode):
            return False

    if isinstance(path, bytes):
        parent = join(path, b'..')
    else:
        parent = join(path, '..')
    try:
        s2 = os.lstat(parent)
    except OSError:
        return False

    dev1 = s1.st_dev
    dev2 = s2.st_dev
    if dev1 != dev2:
        return True     # path/.. on a different device as path
    ino1 = s1.st_ino
    ino2 = s2.st_ino
    if ino1 == ino2:
        return True     # path/.. is the same i-node as path
    return False

@malmaud
Copy link
Contributor Author

malmaud commented May 25, 2015

Alright, I added docs and make it compatible with the Python definition of a mount point. Not sure how to add a test unless a test can be a fixture that actually mounts a volume. Is that possible in Julia's testing framework?

@StefanKarpinski
Copy link
Member

Let's merge this since it seems like reasonable and useful functionality and open a separate issue for figuring out how to test it.

StefanKarpinski added a commit that referenced this pull request May 27, 2015
@StefanKarpinski StefanKarpinski merged commit 1d8e130 into JuliaLang:master May 27, 2015
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.

9 participants