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

Smallish cleanups to docs & unit tests; improve a few tests #3429

Merged
merged 7 commits into from
Jul 30, 2013
Merged

Smallish cleanups to docs & unit tests; improve a few tests #3429

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5dc6489
Cleanups, mainly to unit tests:
peterflynn Apr 5, 2013
045fae6
Cleanups:
peterflynn Apr 12, 2013
f59be5a
Merge remote-tracking branch 'origin/master' into pflynn/unit-test-cl…
peterflynn Jul 23, 2013
7250b96
Fix JSLint/JSHint errors (some of which were on master).
peterflynn Jul 23, 2013
8d94d0d
Merge remote-tracking branch 'origin/master' into pflynn/unit-test-cl…
peterflynn Jul 30, 2013
98f23f2
Code review fixes - tweak docs, cleanup vars, convert for to forEach()
peterflynn Jul 30, 2013
9df7830
Code review: some more small JSDoc fixes
peterflynn Jul 30, 2013
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
49 changes: 19 additions & 30 deletions src/document/DocumentManager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -817,43 +817,32 @@ define(function (require, exports, module) {
* @param {boolean} isFolder True if path is a folder; False if it is a file.
*/
function notifyPathNameChanged(oldName, newName, isFolder) {
var i, path;

// Update open documents. This will update _currentDocument too, since
// the current document is always open.
var keysToDelete = [];
for (path in _openDocuments) {
if (_openDocuments.hasOwnProperty(path)) {
if (FileUtils.isAffectedWhenRenaming(path, oldName, newName, isFolder)) {
var doc = _openDocuments[path];

// Copy value to new key
var newKey = path.replace(oldName, newName);
_openDocuments[newKey] = doc;

keysToDelete.push(path);

// Update document file
FileUtils.updateFileEntryPath(doc.file, oldName, newName, isFolder);
doc._notifyFilePathChanged();

if (!isFolder) {
// If the path name is a file, there can only be one matched entry in the open document
// list, which we just updated. Break out of the for .. in loop.
break;
}
}
CollectionUtils.forEach(_openDocuments, function (doc, path) {
if (FileUtils.isAffectedWhenRenaming(path, oldName, newName, isFolder)) {
// Copy value to new key
var newKey = path.replace(oldName, newName);
_openDocuments[newKey] = doc;

keysToDelete.push(path);

// Update document file
FileUtils.updateFileEntryPath(doc.file, oldName, newName, isFolder);
doc._notifyFilePathChanged();
}
}
});

// Delete the old keys
for (i = 0; i < keysToDelete.length; i++) {
delete _openDocuments[keysToDelete[i]];
}
keysToDelete.forEach(function (fullPath) {
delete _openDocuments[fullPath];
});

// Update working set
for (i = 0; i < _workingSet.length; i++) {
FileUtils.updateFileEntryPath(_workingSet[i], oldName, newName, isFolder);
}
_workingSet.forEach(function (fileEntry) {
FileUtils.updateFileEntryPath(fileEntry, oldName, newName, isFolder);
Copy link
Member Author

Choose a reason for hiding this comment

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

I had to remove this early-exit optimization for now. I doubt it has any discernible impact, but if I'm wrong we can reintroduce it once pull #3117 lands.

Copy link
Contributor

Choose a reason for hiding this comment

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

Now that #3117 has landed, this could use CollectionUtils.some.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think in retrospect I'd actually prefer to leave it using CollectonUtils.forEach(), since it keeps the code a little simpler -- and there doesn't seem to be any real need to optimize here.

Copy link
Contributor

Choose a reason for hiding this comment

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

If there isn't nothing to optimize, then we can leave it like this. If not switching to CollectionUtils.some is as easy as adding return !isFolder inside the if and return false outside.

});

// Send a "fileNameChanged" event. This will trigger the views to update.
$(exports).triggerHandler("fileNameChange", [oldName, newName]);
Expand Down
3 changes: 3 additions & 0 deletions src/editor/EditorManager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -368,6 +368,9 @@ define(function (require, exports, module) {
var editor = document._masterEditor;

if (!editor) {
if (!(document instanceof DocumentManager.Document)) {
throw new Error("_destroyEditorIfUnneeded() should be passed a Document");
}
return;
}

Expand Down
5 changes: 2 additions & 3 deletions src/extensions/default/HTMLCodeHints/unittests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ define(function (require, exports, module) {
CodeHintManager = brackets.getModule("editor/CodeHintManager"),
HTMLCodeHints = require("main");

describe("HTML Attribute Hinting", function () {
describe("HTML Code Hinting", function () {

var defaultContent = "<!doctype html>\n" +
"<html>\n" +
Expand All @@ -49,7 +49,6 @@ define(function (require, exports, module) {
"</body>\n" +
"</html>\n";

var testWindow;
var testDocument, testEditor;

beforeEach(function () {
Expand Down Expand Up @@ -690,5 +689,5 @@ define(function (require, exports, module) {
});


}); // describe("HTML Attribute Hinting"
}); // describe("HTML Code Hinting"
});
23 changes: 12 additions & 11 deletions src/language/LanguageManager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ define(function (require, exports, module) {
/**
* Resolves a language ID to a Language object.
* File names have a higher priority than file extensions.
* @param {!string} id Identifier for this language, use only letters a-z or digits 0-9 and _ inbetween (i.e. "cpp", "foo_bar", "c99")
* @param {!string} id Identifier for this language: lowercase letters, digits, and _ separators (e.g. "cpp", "foo_bar", "c99")
Copy link
Contributor

Choose a reason for hiding this comment

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

Strings are not-nullable by default, so ! is not needed.
Same goes for lines all the other places where it is used as !string of it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd prefer to leave this as-is for now -- we're very inconsistent about flagging nullability at all in most docs, so having it marked explicitly (even if matching the default) seems useful since it indicates someone definitely thought about whether it's nullable or not. If we ever do a big sweep and clean up all our JSDocs, it seems safe to start leaving it unspecified when matching the default... but maybe not just yet.

One of these days I still want to write an extension that runs Closure on your code to give type safety warnings -- sort of a JSLint/Hint on steroids. I think that would make it much easier for us to actually use Closure annotations properly... As it stands, I bet it's hard to find a file in our codebase that doesn't contain at least one incorrect type annotation :-(

Copy link
Contributor

Choose a reason for hiding this comment

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

We are really inconsistent about flagging nullability, but I started to fix them on some files I updated and I know others did too. Not a big thing, we can fix this another time.

It would be nice to have a Closure warning plugin.

* @return {Language} The language with the provided identifier or undefined
*/
function getLanguage(id) {
Expand Down Expand Up @@ -322,7 +322,7 @@ define(function (require, exports, module) {

/**
* Sets the identifier for this language or prints an error to the console.
* @param {!string} id Identifier for this language, use only letters a-z or digits 0-9, and _ inbetween (i.e. "cpp", "foo_bar", "c99")
* @param {!string} id Identifier for this language: lowercase letters, digits, and _ separators (e.g. "cpp", "foo_bar", "c99")
* @return {boolean} Whether the ID was valid and set or not
*/
Language.prototype._setId = function (id) {
Expand Down Expand Up @@ -350,7 +350,7 @@ define(function (require, exports, module) {

/**
* Sets the human-readable name of this language or prints an error to the console.
* @param {!string} name Human-readable name of the language, as it's commonly referred to (i.e. "C++")
* @param {!string} name Human-readable name of the language, as it's commonly referred to (e.g. "C++")
* @return {boolean} Whether the name was valid and set or not
*/
Language.prototype._setName = function (name) {
Expand All @@ -373,7 +373,7 @@ define(function (require, exports, module) {
/**
* Loads a mode and sets it for this language.
*
* @param {string|Array.<string>} mode CodeMirror mode (i.e. "htmlmixed"), optionally with a MIME mode defined by that mode ["clike", "text/x-c++src"]
* @param {string|Array.<string>} mode CodeMirror mode (e.g. "htmlmixed"), optionally with a MIME mode defined by that mode ["clike", "text/x-c++src"]
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be {(string|Array.<string>)} since is an union type.
Same for lines 522, 646 and 647.

* Unless the mode is located in thirdparty/CodeMirror2/mode/<name>/<name>.js, you need to first load it yourself.
*
* @return {$.Promise} A promise object that will be resolved when the mode is loaded and set
Expand Down Expand Up @@ -519,7 +519,7 @@ define(function (require, exports, module) {
/**
* Sets the prefixes to use for line comments in this language or prints an error to the console.
* @param {!string|Array.<string>} prefix Prefix string or an array of prefix strings
* to use for line comments (i.e. "//" or ["//", "#"])
* to use for line comments (e.g. "//" or ["//", "#"])
* @return {boolean} Whether the syntax was valid and set or not
*/
Language.prototype.setLineCommentSyntax = function (prefix) {
Expand Down Expand Up @@ -637,13 +637,14 @@ define(function (require, exports, module) {
/**
* Defines a language.
*
* @param {!string} id Unique identifier for this language, use only letters a-z or digits 0-9, and _ inbetween (i.e. "cpp", "foo_bar", "c99")
* @param {!string} id Unique identifier for this language: lowercase letters, digits, and _ separators (e.g. "cpp", "foo_bar", "c99")
* @param {!Object} definition An object describing the language
* @param {!string} definition.name Human-readable name of the language, as it's commonly referred to (i.e. "C++")
* @param {Array.<string>} definition.fileExtensions List of file extensions used by this language (i.e. ["php", "php3"])
* @param {Array.<string>} definition.blockComment Array with two entries defining the block comment prefix and suffix (i.e. ["<!--", "-->"])
* @param {string|Array.<string>} definition.lineComment Line comment prefixes (i.e. "//" or ["//", "#"])
* @param {string|Array.<string>} definition.mode CodeMirror mode (i.e. "htmlmixed"), optionally with a MIME mode defined by that mode ["clike", "text/x-c++src"]
* @param {!string} definition.name Human-readable name of the language, as it's commonly referred to (e.g. "C++")
* @param {Array.<string>} definition.fileExtensions List of file extensions used by this language (e.g. ["php", "php3"] or ["coffee.md"] - may contain dots)
* @param {Array.<string>} definition.fileNames List of exact file names (e.g. ["Makefile"] or ["package.json]). Higher precedence than file extension.
* @param {Array.<string>} definition.blockComment Array with two entries defining the block comment prefix and suffix (e.g. ["<!--", "-->"])
* @param {string|Array.<string>} definition.lineComment Line comment prefixes (e.g. "//" or ["//", "#"])
* @param {string|Array.<string>} definition.mode CodeMirror mode (e.g. "htmlmixed"), optionally with a MIME mode defined by that mode ["clike", "text/x-c++src"]
* Unless the mode is located in thirdparty/CodeMirror2/mode/<name>/<name>.js, you need to first load it yourself.
*
* @return {$.Promise} A promise object that will be resolved with a Language object
Expand Down
11 changes: 5 additions & 6 deletions src/widgets/Dialogs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,13 +253,12 @@ define(function (require, exports, module) {
* Creates a new general purpose modal dialog using the default template and the template variables given
* as parameters as described.
*
* @param {string} dlgClass A class name identifier for the dialog.
* @param {string=} title The title of the dialog. Can contain HTML markup. If unspecified, the title
* in the JSON file is used unchanged.
* @param {string=} message The message to display in the dialog. Can contain HTML markup. If
* unspecified, the message in the JSON file is used.
* @param {string} dlgClass A class name identifier for the dialog. Typically one of DefaultDialogs.*
* @param {string=} title The title of the dialog. Can contain HTML markup. Defaults to "".
* @param {string=} message The message to display in the dialog. Can contain HTML markup. Defaults to "".
* @param {Array.<{className: string, id: string, text: string>=} buttons An array of buttons where each button
Copy link
Contributor

Choose a reason for hiding this comment

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

I missed a } here... It should be {Array.<{className: string, id: string, text: string}>=}

* has a class, id and text property. The id is used in "data-button-id". It defaults to an Ok button
* has a class, id and text property. The id is used in "data-button-id". Defaults to a single Ok button.
* Typically className is one of DIALOG_BTN_CLASS_*, id is one of DIALOG_BTN_*
* @return {Dialog}
*/
function showModalDialog(dlgClass, title, message, buttons) {
Expand Down
24 changes: 9 additions & 15 deletions test/spec/CSSUtils-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -973,10 +973,10 @@ define(function (require, exports, module) {

// Braces inside string; string inside rule (not inside selector)
css = "a::after { content: ' {' attr(href) '}'; } \n" +
".foo { color:red } \n" +
"a::after { content: \" {\" attr(href) \"}\"; } \n" +
"li::before { content: \"} h4 { color:black }\"; } \n" +
"div { color:green }";
".foo { color:red } \n" +
"a::after { content: \" {\" attr(href) \"}\"; } \n" +
"li::before { content: \"} h4 { color:black }\"; } \n" +
"div { color:green }";
Copy link
Contributor

Choose a reason for hiding this comment

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

I just realized that these new JSLint errors are because we are now using an older version of JSLint that before and checked that these aren't errors on a newer version. So we probably shouldn't fix anymore of these errors and I can revert these changes in #4557.

Copy link
Member Author

Choose a reason for hiding this comment

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

Aha, that explains why no one noticed the errors in this file earlier. Good to know!

Copy link
Contributor

Choose a reason for hiding this comment

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

Noticed it when I saw a similar error in EditorCommandHandlers, which I know that didn't had JSLint errors before.


result = match(css, { tag: "a" });
expect(result.length).toBe(2);
Expand All @@ -999,7 +999,7 @@ define(function (require, exports, module) {
expect(result.length).toBe(1);

css = "@import \"null?\\\"{\"; \n" + // a real-world CSS hack similar to the above case
"div { color: red }";
"div { color: red }";
result = match(css, { tag: "div" });
expect(result.length).toBe(1);

Expand Down Expand Up @@ -1051,9 +1051,9 @@ define(function (require, exports, module) {
expect(result.length).toBe(1);

css = ".foo\n" +
"{\n" +
" color: red;\n" +
"}";
"{\n" +
" color: red;\n" +
"}";
result = match(css, { clazz: "foo" });
expect(result.length).toBe(1);
});
Expand Down Expand Up @@ -1411,29 +1411,23 @@ define(function (require, exports, module) {
var testPath = SpecRunnerUtils.getTestPath("/spec/CSSUtils-test-files"),
CSSUtils,
DocumentManager,
FileViewController,
ProjectManager,
brackets;
FileViewController;

beforeEach(function () {
SpecRunnerUtils.createTestWindowAndRun(this, function (testWindow) {
// Load module instances from brackets.test
brackets = testWindow.brackets;
CSSUtils = testWindow.brackets.test.CSSUtils;
DocumentManager = testWindow.brackets.test.DocumentManager;
FileViewController = testWindow.brackets.test.FileViewController;
ProjectManager = testWindow.brackets.test.ProjectManager;

SpecRunnerUtils.loadProjectInTestWindow(testPath);
});
});

afterEach(function () {
brackets = null;
CSSUtils = null;
DocumentManager = null;
FileViewController = null;
ProjectManager = null;
SpecRunnerUtils.closeTestWindow();
});

Expand Down
40 changes: 6 additions & 34 deletions test/spec/Editor-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ define(function (require, exports, module) {
}

describe("Editor", function () {
var defaultContent = 'Brackets is going to be awesome!\n';
var defaultContent = "Brackets is going to be awesome!\n";
var myDocument, myEditor;

function createTestEditor(content, languageId) {
Expand Down Expand Up @@ -110,42 +110,14 @@ define(function (require, exports, module) {
myEditor._codeMirror.setValue("new content");
expect(changeFired).toBe(true);
});

});

describe("File extension to mode mapping", function () {

it("should switch to the HTML mode for files ending in .html", function () {
// verify editor content
var mode = LanguageManager.getLanguageForPath("c:/only/testing/the/path.html").getMode();
expect(mode).toSpecifyModeNamed("text/x-brackets-html");
});

it("should switch modes for UNIX absolute path", function () {
// verify editor content
var mode = LanguageManager.getLanguageForPath("/only/testing/the/path.css").getMode();
expect(mode).toSpecifyModeNamed(langNames.css.mode);
});

it("should switch modes for relative path", function () {
// verify editor content
var mode = LanguageManager.getLanguageForPath("only/testing/the/path.css").getMode();
expect(mode).toSpecifyModeNamed(langNames.css.mode);
});

it("should accept just a file name too", function () {
// verify editor content
var mode = LanguageManager.getLanguageForPath("path.js").getMode();
expect(mode).toSpecifyModeNamed(langNames.javascript.mode);
});

it("should default to plain text for unknown file extensions", function () {
// verify editor content
var mode = LanguageManager.getLanguageForPath("test.foo").getMode();
it("should set mode based on Document language", function () {
createTestEditor(defaultContent, "html");

// "unknown" mode uses it's MIME type instead
expect(mode).toBe("text/plain");
var htmlLanguage = LanguageManager.getLanguage("html");
expect(myEditor.getModeForDocument()).toBe(htmlLanguage.getMode());
});

Copy link
Member Author

Choose a reason for hiding this comment

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

The tests above were just testing LanguageManager functionality. I simplified & moved those into LanguageManager-test, and in their place added a single test here to make sure the Document's language actually carries through to the Editor's mode (which the old tests never checked).

});

describe("Focus", function () {
Expand Down
49 changes: 43 additions & 6 deletions test/spec/FileIndexManager-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,23 +247,24 @@ define(function (require, exports, module) {

});

it("should handle simultaneous requests without doing extra work", function () { // #330
it("should handle identical simultaneous requests without doing extra work", function () { // #330
var projectRoot,
allFiles1,
allFiles2;

// Open a directory
SpecRunnerUtils.loadProjectInTestWindow(testPath);

var projectRoot;
var promise1, promise2;
var allFiles1, allFiles2;
runs(function () {
projectRoot = ProjectManager.getProjectRoot();
spyOn(projectRoot, "createReader").andCallThrough();

// Kick off two index requests in parallel
promise1 = FileIndexManager.getFileInfoList("all")
var promise1 = FileIndexManager.getFileInfoList("all")
.done(function (result) {
allFiles1 = result;
});
promise2 = FileIndexManager.getFileInfoList("all")
var promise2 = FileIndexManager.getFileInfoList("all") // same request again
.done(function (result) {
allFiles2 = result;
});
Expand All @@ -282,6 +283,42 @@ define(function (require, exports, module) {
});
});

it("should handle differing simultaneous requests without doing extra work", function () { // #330
var projectRoot,
allFiles1,
allFiles2;

// Open a directory
SpecRunnerUtils.loadProjectInTestWindow(testPath);

runs(function () {
projectRoot = ProjectManager.getProjectRoot();
spyOn(projectRoot, "createReader").andCallThrough();

// Kick off two index requests in parallel
var promise1 = FileIndexManager.getFileInfoList("all")
.done(function (result) {
allFiles1 = result;
});
var promise2 = FileIndexManager.getFileInfoList("css") // different request in parallel
.done(function (result) {
allFiles2 = result;
});

waitsForDone(promise1, "First FileIndexManager.getFileInfoList()");
waitsForDone(promise2, "Second FileIndexManager.getFileInfoList()");
});

runs(function () {
// Correct response to both promises
expect(allFiles1.length).toEqual(8);
expect(allFiles2.length).toEqual(3);

// Didn't scan project tree twice
expect(projectRoot.createReader.callCount).toBe(1);
});
});

it("should match a specific filename and return the correct FileInfo", function () {
// Open a directory
SpecRunnerUtils.loadProjectInTestWindow(testPath);
Expand Down
Loading