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

Fixed: #1997 and #1397 #2685

Merged
merged 10 commits into from
Jan 29, 2013
Merged

Fixed: #1997 and #1397 #2685

merged 10 commits into from
Jan 29, 2013

Conversation

WebsiteDeveloper
Copy link
Contributor

Fixed: #1397 Code Cleanup: remove additional keys mechanism and
Fixed: #1997 Avoid use of then() on promises

@ghost ghost assigned dangoor Jan 28, 2013
@dangoor
Copy link
Contributor

dangoor commented Jan 28, 2013

Thanks for the code contribution! I've taken a peek at the code, but I also tried running the tests...

I see a whole bunch of unit test failures. (66 in EditorCommandHandlers, for example). It might be something simple. Have you tried running the tests? (Make sure you hit Show Developer Tools and have caching turned off via the gear menu in the Chrome developer tools, just to be sure you're not running cached code...)

Thanks again. Looks like a good start.

@WebsiteDeveloper
Copy link
Contributor Author

The failures were due to the initialisation of an editor object with the additionalKeys param in the unit tests i cleaned this up and all my unit tests pass so it should work now

@@ -54441,7 +54441,7 @@ define('editor/EditorManager',['require','exports','module','file/FileUtils','co
function _createEditorForDocument(doc, makeMasterEditor, container, range, additionalKeys) {
var mode = EditorUtils.getModeFromFileExtension(doc.file.fullPath);

return new Editor(doc, makeMasterEditor, mode, container, additionalKeys, range);
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't modify this file -- it's an old copy of the Brackets source that we use as a performance testcase. We want it frozen so we can compare performance results over time.

@WebsiteDeveloper
Copy link
Contributor Author

@peterflynn I restored the file

@dangoor
Copy link
Contributor

dangoor commented Jan 29, 2013

I just spotted one more editor that slipped through: HTMLCodeHints/unittests.js has a call with the additional keys parameter (extension tests don't run as part of the main suite, but you can run them by clicking on Extensions in the test runner).

With that change, I think the #1397 fixes look good to me. I'll read through the promises changes now.

@dangoor
Copy link
Contributor

dangoor commented Jan 29, 2013

The .then changes look fine, too. I spotted one of those that's missing: LiveDevelopment/Agents/RemoteAgent.js has one on line 94.

@WebsiteDeveloper
Copy link
Contributor Author

@dangoor I changed the things you remarked
@peterflynn ready for final review

@peterflynn
Copy link
Member

@WebsiteDeveloper - thanks for the fix! I'll let Kevin take it from here :-)

@dangoor
Copy link
Contributor

dangoor commented Jan 29, 2013

@WebsiteDeveloper thanks for the updates, everything looks good and I'll merge it as soon as our automated build server is in the green again.

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.

Avoid use of then() on promises Code Cleanup: remove additional keys mechanism
3 participants