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

Empty pref file is an empty object, not an error. #7263

Merged
redmunds merged 2 commits into from
Mar 21, 2014
Merged

Empty pref file is an empty object, not an error. #7263

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4c737e3
Fix for #7220: empty pref file is an empty object, not an error.
dangoor Mar 20, 2014
8a33a5d
Fix the regex based on review comment.
dangoor Mar 21, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions src/preferences/PreferencesBase.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,10 +175,15 @@ define(function (require, exports, module) {

self._lineEndings = FileUtils.sniffLineEndings(text);

try {
result.resolve(JSON.parse(text));
} catch (e) {
result.reject(new ParsingError("Invalid JSON settings at " + path + "(" + e.toString() + ")"));
// If the file is empty, turn it into an empty object
if (/^\w*$/.test(text)) {
Copy link
Copy Markdown
Contributor

@redmunds redmunds Mar 21, 2014

Choose a reason for hiding this comment

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

I don't think this regex is what you want. This will match an empty file (due to the *), but will also match a file with only [A-Za-z0-9_] chars, which is not what you want.

To match empty or only whitespace, I think it should be /^\s*$/.

Copy link
Copy Markdown
Member

@peterflynn peterflynn Mar 21, 2014

Choose a reason for hiding this comment

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

Ooh, good catch @redmunds! Can't believe I didn't spot that :-(

Copy link
Copy Markdown
Contributor Author

@dangoor dangoor Mar 21, 2014

Choose a reason for hiding this comment

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

Ugh, ugh, ugh. Thanks @redmunds, I must be getting rusty at regular expressions (is that even possible? I used to program in Perl!)

result.resolve({});
} else {
try {
result.resolve(JSON.parse(text));
} catch (e) {
result.reject(new ParsingError("Invalid JSON settings at " + path + "(" + e.toString() + ")"));
}
}
});
} else {
Expand Down
Empty file added test/spec/PreferencesBase-test-files/empty.json
View file
Open in desktop
Empty file.
20 changes: 18 additions & 2 deletions test/spec/PreferencesBase-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1067,8 +1067,9 @@ define(function (require, exports, module) {
});

describe("File Storage", function () {
var settingsFile = FileSystem.getFileForPath(testPath + "/.brackets.json"),
newSettingsFile = FileSystem.getFileForPath(testPath + "/new.prefs"),
var settingsFile = FileSystem.getFileForPath(testPath + "/.brackets.json"),
newSettingsFile = FileSystem.getFileForPath(testPath + "/new.prefs"),
emptySettingsFile = FileSystem.getFileForPath(testPath + "/empty.json"),
filestorage,
originalText;

Expand Down Expand Up @@ -1207,6 +1208,21 @@ define(function (require, exports, module) {
}]);
});
});

it("is fine with empty preferences files", function () {
var filestorage = new PreferencesBase.FileStorage(emptySettingsFile.fullPath),
promise = filestorage.load();

waitsForDone(promise, "loading empty JSON file");
runs(function () {
promise.then(function (data) {
expect(data).toEqual({});
})
.fail(function (error) {
expect("There should have been no error").toEqual("");
});
Copy link
Copy Markdown
Member

@peterflynn peterflynn Mar 20, 2014

Choose a reason for hiding this comment

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

I don't think you even need the fail() block since waitsForDone() already verifies that it didn't fail.

I reviewed the rest of this btw and it looks good... so @redmunds / @dangoor if you don't think the above extra bit of code is a big deal feel free to merge now.

Copy link
Copy Markdown
Contributor

@redmunds redmunds Mar 21, 2014

Choose a reason for hiding this comment

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

Yes, I think the waitsForDone() is enough, so the fail() should be removed.

Copy link
Copy Markdown
Contributor Author

@dangoor dangoor Mar 21, 2014

Choose a reason for hiding this comment

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

Good point. I'll remove that.

});
});
});
});
});
⚡