Re: Polished FileSystem API proposal

On Tue, Jul 16, 2013 at 8:56 AM, Eric U <ericu@google.com> wrote:
> On Mon, Jul 15, 2013 at 5:58 PM, Jonas Sicking <jonas@sicking.cc> wrote:
>> On Mon, Jul 15, 2013 at 4:47 PM, Eric U <ericu@google.com> wrote:
>>>>> What's the purpose of DestinationDict?  It seems redundant.
>>>>
>>>> There's basically three potential ways to specify a copy/move destination:
>>>>
>>>> Specify a new name within the current directory.
>>>> Specify a new directory but keep the current name.
>>>> Specify a new directory as well as a new name within that directory.
>>>>
>>>> We obviously don't have to support them all, for example you could
>>>> imagine requiring a two-step process in some cases. But we decided to
>>>> try to support all three as a single operation for now.
>>>>
>>>> Coming up with a syntax for this is somewhat tricky. In C++ you'd
>>>> simply have three overloads that take the different combinations, but
>>>> that doesn't feel very "javascripty".
>>>>
>>>> Another is to use optional arguments like:
>>>>
>>>> move(..., Directory? destDir, optional DOMString destName);
>>>>
>>>> would work. That means that the three options would look like:
>>>>
>>>> dir.move("srcfile", null, "newName");
>>>> dir.move("srcfile", destDir);
>>>> dir.move("srcfile", destDir, "newName");
>>>
>>> dir.move("srcfile", "dir/and/new/name");
>>>
>>> Granted, that doesn't help you if you're holding a Directory object
>>> and don't have a way to generate a relative path to it. But perhaps
>>> that's not a big lack?  You can always munge paths yourself.
>>>
>>> Obviously we went with the way you'd see as iffy.
>>>
>>>> Unfortunately the first one there looks pretty iffy. So instead we
>>>> went with a union which results in syntax like:
>>>>
>>>> dir.move("srcfile", "newName");
>>>> dir.move("srcfile", destDir);
>>>> dir.move("srcfile", { dir: destDir. name: "newName" });
>>>
>>> In this syntax, could you say it this way?
>>>
>>> dir.move("srcfile", { dir: "dirName". name: "newName" });
>>
>> No, but you could say
>>
>> dir.move("srcfile", "dirName/newName");
>>
>> Or you can use .get() to grab the directory object named "dirName" and use that.
>
> So what can't you do without the DestinationDict?  You can rename the
> file without moving it to a new dir, you can move it to a new dir
> without renaming it, and you can rename and move it using
> "dirName/newName" as the second argument.

Only if the new destination directory is a subdir of the current directory.

/ Jonas

>>>>> You have MakeFileOptions.overwriteIfExists for createFile, but
>>>>> CreateMode { createIfNeeded | dontcreate } for openWrite; it feels
>>>>> like those should share a set of flags.  The options should probably
>>>>> be {createIfNeeded, dontCreate, failIfExists}, where failIfExists
>>>>> implies createIfNeeded and is the standard POSIX paradigm
>>>>> [O_CREAT|O_EXCL].  It's useful for coordination between multiple pages
>>>>> in the same origin, or the same page loaded twice.
>>>>
>>>> So you're basically proposing (modulo naming):
>>>>
>>>> createFile("name", { mode: "createIfNeeded" });
>>>> Creates the file if it doesn't exist. Overwrites it if it does.
>>>
>>> Is the idea that createFile will overwrite an existing file's
>>> contents, even if you don't supply any, e.g. nuking a file down to 0
>>> bytes?
>>
>> Yes.
>>
>>> That might be a bit surprising.  Would you require
>>> MakeFileOptions.data?
>>
>> In the current proposal the function is called "createFile" and if you
>> want it to overwrite what's there you have to explicitly opt in to
>> that using
>>
>> createFile("name", { overwriteIfExists: true });
>>
>> so I don't think the current proposal is that surprising. Especially
>> since creating files usually will either error or remove what's there.
>> I'd like to keep that clarity.
>>
>>>> createFile("name", { mode: "dontCreate" });
>>>> Error if the file doesn't exist. Overwrites it if it does.
>>>
>>> I'm not sure why you'd ever call it this way.  Perhaps disallow it for
>>> now?  Otherwise it might become a standard hack for some bizarre
>>> file-based locking paradigm.
>>>
>>>> createFile("name", { mode: "failIfExists" });
>>>> Creates the file if it doesn't exist. Error it if it does.
>>>>
>>>> openWrite("name", "createIfNeeded");
>>>> Creates and opens the file if it doesn't exist. Opens it if it does.
>>>>
>>>> openWrite("name", "dontCreate");
>>>> Error if the file doesn't exist. Opens it if it does.
>>>>
>>>> openWrite("name", "failIfExists");
>>>> Creates and opens the file if it doesn't exist. Error it if it does.
>>>>
>>>> Is this correct? While I agree it's consistent, the number of
>>>> combinations here is a bit mind boggling. It took me quite a while to
>>>> write all of these out and make sure they are correct. The
>>>> createFile("dontCreate") combination is especially confusing.
>>>
>>> I'd skip that one; I think the other 5 make sense, and are better than
>>> having two different paradigms to say almost the same thing depending
>>> on which function you call [boolean vs. string argument].  If you
>>> decide you don't want failIfExists, that makes the list even simpler.
>>
>> I think the 4 modes that are important to support are:
>>
>> Create new file and error if there's already an existing one
>> Create new file and overwrite if there's already an existing one
>> Open for writing and create if doesn't exist
>> Open for writing and error if doesn't exist
>>
>> The behavior that needs to be defined for the create function is what
>> to do if the file already exists, because generally you aren't
>> expecting a file to exist when you are requesting it to be created.
>> The behavior that needs to be defined for the open function is what to
>> do if the function does not exist.
>>
>> So I don't know that trying to align them too much is going to work
>> very well. I do totally agree that the current setup is not good
>> enough though.
>>
>> How about:
>>
>> createFile("name", { ifExists: "overwrite" });
>> createFile("name", { ifExists: "fail" });
>> openWrite("name", { ifNotExist: "create" });
>> openWrite("name", { ifNotExist: "fail" });
>>
>> then we can duke it out what the defaults should be :)
>>
>>>>> If you don't allow ".." to traverse directories, please define using
>>>>> it as a path segment as an error, to allow for future expansion and to
>>>>> catch bugs for folks who didn't read the spec.
>>>>
>>>> Definitely.
>>>>
>>>>> openForWrite or openForWriting, while longer, would be clearer than
>>>>> openWrite.  Heck, why not just "write"?
>>>>
>>>> I tried out just "write" for a while, but it seemed strange that
>>>> myDir.write() actually started writing to a file.
>>>
>>> Doesn't seem all that worse than myDir.openForWriting() to me, but is
>>> openWrite better in any way?
>>> Sure, it doesn't sound like you're opening the directory for writing,
>>> but that's because it doesn't make any sense at all.
>>
>> I've consistently gotten feedback that authors prefer shorter names
>> over absolute clearness. So I still think openWrite wins over
>> openForWriting. I could be convinced that "write" is better, but so
>> far it doesn't seem like there's more consensus for that.
>>
>> / Jonas

Received on Tuesday, 16 July 2013 17:05:52 UTC