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

New FileSystem API #5797

Merged
merged 206 commits into from
Nov 8, 2013
Merged

New FileSystem API #5797

merged 206 commits into from
Nov 8, 2013

Conversation

gruehle
Copy link
Member

@gruehle gruehle commented Oct 31, 2013

This pull request adds a new FileSystem API that replaces NativeFileSystem, FileIndexManager and direct calls to brackets.fs.* functions.

This pull request changes the API, but does not add any significant new features or performance gains. These will be done in subsequent pull requests.

This is a big change. It will break some extensions. However, the breakages are minimized through deprecation "shims" for many of the commonly used APIs that have been removed.

See this Brackets-Dev forum thread for an introduction to the changes.

See the FileSystem and FileSystem API Migration wiki pages for more details.

Sign-offs:

@iwehrman
Copy link
Contributor

iwehrman commented Nov 7, 2013

All changes from the review comments pushed.

}
callback(err, stat);
} catch (ex) {
console.warn("Unhandled exception in callback: ", ex);
Copy link
Member

Choose a reason for hiding this comment

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

@iwehrman I'd rather us not do a catch-and-log if we don't need to, since it breaks the debugger "break on uncaught exceptions" feature. In this case it seems unneeded since the only thing that comes afterward is in a finally block anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

I forgot about the "break on uncaught exceptions" use case. I'll change all these try-catch-finally blocks in which the catch only contains a console.warn to just try-finally blocks.

}
callback(err); // notify caller
} finally {
this._fileSystem._endWrite(); // unblock generic change events
Copy link
Member

Choose a reason for hiding this comment

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

Ditto on the catch

@peterflynn
Copy link
Member

Sorry for adding a bunch of comments saying I was fixing stuff that's already been fixed... dunno how I got so far out of sync from the tip of the branch. One of my git fetches must have filed without me noticing the error message, or something.

@peterflynn
Copy link
Member

Holy crap, it's actually ready to land. Here goes nothing! 🙈   🙏

peterflynn added a commit that referenced this pull request Nov 8, 2013
@peterflynn peterflynn merged commit 598007f into master Nov 8, 2013
@MiguelCastillo
Copy link
Contributor

Oh man... Let's see how this one breaks my extensions 🎲

@peterflynn
Copy link
Member

Boom! The eagle has landed!

rexnuke

@iwehrman
Copy link
Contributor

iwehrman commented Nov 8, 2013

thumbs up

@peterflynn
Copy link
Member

High five, Ian! 🙌

@njx
Copy link
Contributor

njx commented Nov 8, 2013

high five

@larz0
Copy link
Member

larz0 commented Nov 8, 2013

high20five_xlarge

@lkcampbell
Copy link
Contributor

sparky06

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.

10 participants