Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

NativeFileSystem documentation and examples (#1052) #2063

Merged
merged 10 commits into from
Dec 14, 2012
Merged

NativeFileSystem documentation and examples (#1052) #2063

merged 10 commits into from
Dec 14, 2012

Conversation

jbalsas
Copy link
Contributor

@jbalsas jbalsas commented Nov 6, 2012

Hi,

I was going through NativeFileSystem the other day and ran across #1052

This pull request should address those concerns. Additionally, it removes some unused requires with NativeFileSystem.

As pointed out in #2058, some of the APIs are not consitent with the current specs. The jsdocs here reflect the specs, and would need to get updated with that information.

@ghost ghost assigned peterflynn Nov 6, 2012
@ghost ghost assigned jasonsanjose Dec 11, 2012
@jasonsanjose
Copy link
Member

Assigned to me. @jbalsas Now that the other NativeFileSystem changes are merged, do you want to update this pull request or should I close this?

@jbalsas
Copy link
Contributor Author

jbalsas commented Dec 12, 2012

Sure! Let me merge with master first and then review and update this before you take a look.

@jbalsas
Copy link
Contributor Author

jbalsas commented Dec 12, 2012

@jasonsanjose Merged and updated. All yours ;)

* NativeFileSystem.DirectoryEntry().getDirectory(path, {create: true});
*
* - CHECK IF A FILE OR FOLDER EXISTS
* NativeFileSystem.requestNativeFileSystem(path
Copy link
Member

Choose a reason for hiding this comment

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

I think resolveNativeFileSystemPath is a more appropriate example. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, makes sense. This wasn't implemented by the time I added the examples, though. Will switch to resolveNativeFileSystemPath here.

/**
* Copies this Entry to a different location on the file system.
* @param {!DirectoryEntry} parent The directory to copy the entry to
* @param {!string=} newName The new name of the entry. If not specified, defaults to the current name
Copy link
Member

Choose a reason for hiding this comment

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

Remove !

@jasonsanjose
Copy link
Member

Initial review complete. Thanks for taking this one.

@jbalsas
Copy link
Contributor Author

jbalsas commented Dec 14, 2012

@jasonsanjose I'll dive into this later today. Just need one clarification.

From the Closure Compiler docs, ! stands for Non-nullable type which Indicates that a value is type A and not null. This aligns with the w3c specs where the error callbacks are defined as optional, non-nullable functions.

Is it possible that we're using ! throughout the docs to mark required arguments instead of non-nullable? If so... should we stick to that?

Furthermore, if we stick to the meaning of ! in the Closure Compiler, Functions and all value types (boolean, number, and string) are non-nullable by default so we could even remove all instances of ! found along functions.

What do you think?

@jasonsanjose
Copy link
Member

Yep, we're using ! wrong. I'll file a separate bug to resolve that.

You're right that it seems redundant to add ! when they are non-nullable by default. I think it's ok to remove ! in that case.

@jbalsas
Copy link
Contributor Author

jbalsas commented Dec 14, 2012

I think I've addressed all your comments. Ready for a second pass.

@jasonsanjose
Copy link
Member

Looks great. I filed #2360 to cover the incorrect annotations. Merging.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants