Removing love.filesystem.exists

Issue #641 resolved
created an issue

I propose that love.filesystem.exists be removed because...

  1. It's redundant.
  2. It's not very useful.
  3. It's frequently misused.

love.filesystem.exists('item') is a shortcut for love.filesystem.isFile('item') or love.filesystem.isDirectory('item'). I don't think it's worth having though. There is a single use I can think of for having it, which is for when you want to save a file or make a directory, but you don't want to overwrite anything, so you check if something with that name exists. I don't think this use case is common enough that the shortcut is needed.

love.filesystem.isFile('item') or love.filesystem.isDirectory('item') is actually quite elegant I think, much like ParticleSystem:getCount() == ParticleSystem:getBufferSize() as opposed to ParticleSystem:isFull(). In both cases, redundancy is avoided, and the potential but somewhat uncommon needs for finding whether anything with a name exists or if a ParticleSystem is full is possible by using the functions available.

It's also misused. Just then I grepped some projects, and it appeared a few times, but in all the places it was found it would have made more sense to use isFile or isDirectory, lest the game try to open a directory as a file, or treat a file as a directory.

There's also the minor aesthetic issue which is that it returns a single boolean value but its name doesn't start with "is" or "has".

Comments (17)

  1. hahawoo reporter

    What are some use cases?

    If you're reading a file, you'll want to use isFile, if you're reading from a directory, you'll want to use isDirectory, so I can't think of why it would be useful for reading.

    If you're writing something like a save game, and you want to overwrite the previous save if it exists, and you want it robust so it doesn't fail silently if, say, the user creates a folder with the same name as the save file, you could check the return value of love.filesystem.write, and respond accordingly.

    ok = love.filesystem.write('save', savedata) 

    Or maybe it is some kind of editor, where the user creates files with names of their choice, and are storing files in the save directory they don't want overwritten.

    Let's say the user enters a name of a file to save or directory to create, but there is already a file/directory with that name. The program should probably respond appropriately based on whether the desired name to save to is a file or a folder. If they're saving a file and their chosen name is also file, you might want to ask "do you want to overwrite?", or if it's a folder you might want to open the directory or give an error message.

    I'm not saying that there are no use cases... I'm just saying that I can't think of any. :P But I'm sure there are some out there.

    When I said it's "redundant", I meant "the same functionality can be achieved through other mechanisms".

    There are two main kinds of "redundancy" in the API the way I think of it:

    • There are things which are "redundant" but are there for performance reasons, like SpriteBatches, ParticleSystems, etc.
    • And there are what I call "shortcuts", things which are simple to do with other mechanisms but you'd want to use them all the time, like

    love.filesystem.exists falls into the "shortcut" category, but would it be used all the time? I'm not so sure. It also seems to be frequently used when isFile or isDirectory would be more appropriate. (I'd hypothesise that this misuse isn't a coincidence, and it may be due to it being redundant and not having frequently occurring use cases.)

  2. Somepotato NA

    There's no point in removing 'shortcuts'. I really don't see why you'd want this removed, none of your points make much sense TBH. There's no sense in overcomplicating this. If you had an actual use for it and it's removed, wouldn't that make you mad?

  3. hahawoo reporter

    Nah, I'd say I'd be pretty happy, I'd probably laugh and think "oh, so there was a use case after all!" and reflect on how the handful of extra keystrokes I just made for this rare scenario were more than worth the cost saved by removing a redundant, misused, and rarely needed function. ;)

    But, maybe I'm wrong! Maybe there is some obvious, frequently occurring use that I've failed to see which totally justifies this function's existence (heh!). If this is the case, please do let me know what that use is!

  4. Seppi

    removing love.filesystem.exists and telling folks to use love.filesystem.isFile and love.filesystem.isDirectory seems like it would cause more stress for folks who are looking for a simple way to detect a file/directory.

    I use it quite frequently in the vapor project.

  5. hahawoo reporter

    But what's the commonly occurring use case where people want a simple way to detect a file/directory?

    I looked through Vapor, and I found 8 occurrences of love.filesystem.exists, but they all check for files, so why not use love.filesystem.isFile?

    • settings.lua, line 10: If there is a folder (and not a file) with the same name as the the value of settings.file, the next line will try to read it as a file, and the program crashes. This is scenario is completely unlikely of course, but why not use isFile?
    • ui.lua, lines 23, 26, 107, 426: These lines check if files exist. Why not use isFile?
    • vapor.lua, lines 47, 51: These lines check if files exist, and tries to read a file it checked on line 52.
    • state_load.lua, line 44: This line checks if a file exists.
  6. Seppi

    it might be a terminology thing. I come from a heavy PHP background, so this is the first thing I fall back on:

    Perhaps instead of love.filesystem.isFile we could have love.filesystem.fileExists and love.filesystem.isDirectory could be love.filesystem.directoryExists.

    As you're right, I probably should be using isFile, but in the use case, vapor should never be making directories instead. I suppose that's a shitty reason, but "exists" as a boolean is the first thing I think of.

  7. Somepotato NA

    The goal shouldn't be to remove functionality that some people might find useful. Like Love has done many times in the past which, put bluntly, makes plenty of people angry. The goal should be to add features instead of questionably removing features that some people might use. Even if you feel it may not be important, someone out there might have a use case for it. I don't see much point in shooting them down because you argue that you find no use for it.

  8. Alex Szpakowski

    The question is: what are the use cases for love.filesystem.exists over love.filesystem.isFile / love.filesystem.isDirectory in practice, and are they common enough that love.filesystem.isFile(name) or love.filesystem.isDirectory(name) needs love.filesystem.exists(name) as a shortcut function?

    I don't really like keeping functions just because they were around in a previous version and for no other reason. They should have an actual purpose that makes sense in a real project. It's often the case that a function will become redundant over time because of the addition of more useful functions that better cover people's use cases.

  9. Seppi

    After seeing this changeset:

    I'm going to change my stance on this. While a paranoid programmer might want to check to see if a file or a directory of the same name exists, in most cases they are looking to see if a directory exists or if a file exists, but they are not checking both.

    I'm going to vote that love.filesystem.exists is cruft, and ought to be removed to promote brevity in the love API.

    A seasoned programmer who really "needs" love.filesystem.exists can simply write their own in a few lines of Lua.

  10. hahawoo reporter

    (Reopening for discussion, feel free to swiftly reclose. :P)

    A couple of things:

    1) Does love.filesystem.isFile return true for a symlinked directory?

    Briefly looking through the PhysFS 2.1 docs, it seems like there's no way to see if a symlink points to a file or directory, is this right?

    PHYSFS_exists is true on a symlink if PHYSFS_permitSymbolicLinks(1) has been called, so currently, with PhysFS 2.1 and symlinks permitted, it would seem love.filesystem.isFile returns true on a symlinked directory, because both PHYSFS_exists(file) and !isDirectory(file) are true, right? This seems like unexpected behaviour for LOVE to have.

    So shouldn't love.filesystem.isFile be like isDirectory and isSymlink?

    bool Filesystem::isFile(const char *file) const
        if (!PHYSFS_isInit())
            return false;
    #ifdef LOVE_USE_PHYSFS_2_1
        PHYSFS_Stat stat = {};
        if (PHYSFS_stat(filename, &stat))
            return stat.filetype == PHYSFS_FILETYPE_REGULAR;
            return false;
        return PHYSFS_exists(file) && !isDirectory(file);

    2) Maybe LOVE could make the behavior consistent regardless of the PhysFS version? Basically by love.filesystem.isFile and love.filesystem.isDirectory returning false on symlinks?

    This seems bad because it's removing the ability to see if a symlink is a file or a directory but it seems good because it makes LOVE's behaviour the same on different operating systems (assuming different operating systems still use different versions of PhysFS).

    bool Filesystem::isDirectory(const char *path) const
        // etc.
    #ifdef LOVE_USE_PHYSFS_2_1
        // etc.
        return PHYSFS_isDirectory(path) != 0 && PHYSFS_isSymbolicLink(path) == 0;
    bool Filesystem::isFile(const char *path) const
        // etc.
    #ifdef LOVE_USE_PHYSFS_2_1
        // etc.
        return PHYSFS_exists(path) && !PHYSFS_isDirectory(path) == 0 && PHYSFS_isSymbolicLink(path) == 0;

    2.1) If the change above is made, then love.filesystem.exists could be removed again?

    I guess now it's a shortcut for

    love.filesystem.isFile(path) or
    love.filesystem.isDirectory(path) or

    which is more than before, but I still can't think of a compelling use case.

  11. Alex Szpakowski

    I think I might turn those functions into a love.filesystem.getType(path) or even a more general love.filesystem.getInfo(path) once PhysFS 2.1/3.0 is released – which might be pretty soon.

  12. Log in to comment