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

File.writeFile's options has 'replace' and 'append' inverted, breaking the function #789

Closed
wwoods opened this issue Nov 11, 2016 · 3 comments

Comments

@wwoods
Copy link

wwoods commented Nov 11, 2016

Here, ionic-native sets exclusive = options.replace: https://github.com/driftyco/ionic-native/blob/master/src/plugins/file.ts#L682

And here, the Cordova plugin rejects an existing file if exclusive is set: https://github.com/apache/cordova-plugin-file/blob/master/src/android/LocalFilesystem.java#L119

ionic-native has its options flipped, should be exclusive = !options.replace.

Another error - the 'append' feature is broken: https://github.com/driftyco/ionic-native/blob/master/src/plugins/file.ts#L681 should have create: !options.append.

Other methods in this file seem to support the replace flag appropriately.

wwoods added a commit to wwoods/ionic-native that referenced this issue Nov 16, 2016
Prior to this patch, `File.writeFile` has the `replace` and `append`
options flipped; setting `replace: true` would block the file from being
replaced, and setting `append: true` would always create a new file and
never append.
@ihadeed
Copy link
Collaborator

ihadeed commented Nov 23, 2016

There was a recent patch that was not included in your fork so I think you didn't see it yet. The current code is as follows:

    const getFileOpts: Flags = {
      create: !('create' in options) || options.create,
      exclusive: !!options.replace
    };

I'm not sure what is a better option to use, create or append. They both make sense in this situation. We can use them both and prioritize one of them. (create: ((create in options) && !!options.create) || !options.append)`)

As for the exclusive option, you're right. I can see here https://github.com/apache/cordova-plugin-file/blob/master/src/android/LocalFilesystem.java#L140-L145 if the file exists and we have both create and exclusive set to true, the operation will fail.

@wwoods
Copy link
Author

wwoods commented Nov 23, 2016

I actually did see that code, but it's broken, for a number of reasons.

First, with the current code, options.replace is still inverted. It should be the exact opposite (still) of what it is: exclusive = !options.replace.

Second, I think {append: true}, the old option, is much clearer than {create: false}. {create: false} is a passive statement saying not to create a file, but it doesn't make clear what should happen instead (append? replace bytes as they're written?). On the other hand, if I see File.writeFile(..., {append: true}), I know what's happening (a new file is not being created, instead I'm expecting to append. If the file didn't exist, I'm creating an empty file and then appending to that).

Third, and my pull request was guilty of this too (I didn't want to make unnecessary changes but rather fix what was there), I think that WriteOptions should have exclusive instead of replace.

Every other file IO library I've seen deals with this the same way: the default mode of operation when writing a file is to create a new file if there wasn't one, or if there was, replace it and overwrite. Supporting append and exclusive as options makes more sense than create and replace, both of which are the default modes of operation. Having boolean options whose names are the default behaviors, but are used to turn off those behaviors, is not intuitive. Generally, optional boolean flags should default to false but can be set to true by the caller - more opt-in behavior, less opt-out.

@ihadeed
Copy link
Collaborator

ihadeed commented Nov 24, 2016

Yeah that makes sense. Just pushed a commit to fix this.

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

No branches or pull requests

2 participants