Continued Language API work #2980

jasonsanjose · 2013-02-27T21:28:45Z

Supercedes #2979.

@DennisKehrig I merged with master to resolve #2978

…, defineLanguage now returns a promise that resolves once the mode has been loaded and set, instead of returning the language directly. To make things more consistent both within LanguageManager and with the rest of the code base, chaining support has been removed.

…clusively through getters and setters Adjust the unit tests accordingly Added documentation to Language's properties Refined the comment about our special HTML MIME mode Renamed fileExtensionsToLanguageMap to fileExtensionToLanguageMap

Call result.reject in a few places instead of throwing exceptions

Conflicts: test/spec/Editor-test.js

jasonsanjose · 2013-02-27T21:30:53Z

test/spec/LanguageManager-test.js

+                }
+            }
+
+            return { promise: promise, language: language };


From @DennisKehrig

For convenience, return both a promise and a language.
Maybe this is a good idea for LanguageManager.defineLanguage(), too?
Or maybe we want to go in the opposite direction, i.e. make sure that a language is only registered (via ID, mode or file extensions) once it's fully specified. I.e. we'd wait with registering it until just before we resolve the promise in defineLanguage to make this basically an atomic operation and not have "incomplete" languages lying around.

I like the idea of not registering it at all until the promise is resolved (that is, until the mode is loaded). That would fix #2962 too.

Agreed. I'll write up the fix.

I'm curious on how you will rewrite the unit tests to deal with that, would you just test once that the promise is resolved and then rely on it in later tests?

Yep. There's a new property ready promise on LanguageManager that checks when all the default languages are loaded. Each test was also updated to wait for the promise when calling defineLanguage.

peterflynn · 2013-02-27T22:45:17Z

src/language/LanguageManager.js

-        // Hence we use _ instead of "." since this makes it easier to parse a file name containing a language ID
-        if (!id.match(/^[a-z]+(\.[a-z]+)*$/)) {
+        // Hence we use "_" instead of "." since the latter often has special meaning
+        if (!id.match(/^[a-z]+(_[a-z]+)*$/)) {


I still don't think we should be so strict about ids. We don't even allow capital letters here... For Command ids, we encourage package-style naming but don't enforce anything specific. I'm still unsure why that's not good enough for Language ids too.

From @DennisKehrig's comment in the original pull request #2844 (comment), he proposes hypothetical jQuery events on a language. If the language has a dot, it's treated as a namespace, e.g.

$(LanguageManager).on("myLanguageId.myNamespace")

He makes a few other points about restricting the use of a dot. I think capital letters should be ok though. I'll add that.

Capital letters are okay, though we lose the ability to convert from foo_bar to FooBar and back, should we ever want/need that.

jasonsanjose · 2013-02-28T00:27:58Z

Addressing the bullets from #2968

We always resolve the promise now, so that addresses my comment. I added error checking for an empty mode string.
Added capital letters. I don't feel strongly about the other restrictions. @DennisKehrig fixed the regex.
@DennisKehrig fixed
Changed to Object.keys
@DennisKehrig fixed
@jasonsanjose fixed
@DennisKehrig updated comment
@DennisKehrig fixed
@DennisKehrig fixed
@DennisKehrig fixed

The additional comments:

"rename" event is still not addressed, but I thought you referenced this in another bug earlier today?
haxe was fixed by @TomMalbran

jasonsanjose · 2013-02-28T00:31:57Z

Changes pushed. @peterflynn ready to review.

DennisKehrig · 2013-02-28T10:41:11Z

@jasonsanjose The rename issue is related to #2961

jasonsanjose · 2013-02-28T20:47:44Z

@peterflynn ready to review

peterflynn · 2013-03-01T03:00:45Z

src/language/LanguageManager.js

- * To find out existing MIME modes, search for "CodeMirror.defineMIME" in thirdparty/CodeMirror2/mode
- * For instance, C++, C# and Java all use the clike mode.
+ * To find existing MIME modes, search for "CodeMirror.defineMIME" in thirdparty/CodeMirror2/mode
+ * For instance, C++, C# and Java all use the clike (C-like) mode.


I think this would be clearer if it read "...all use the "clike" (C-like) mode with different settings and a different MIME name."

peterflynn · 2013-03-01T03:10:39Z

Crap, gotta run to dinner -- will finish review in a couple of hours. Sorry about the delay!

peterflynn · 2013-03-01T07:20:48Z

src/language/LanguageManager.js

-
-        // Since setting the mode is asynchronous when the mode hasn't been loaded yet, offer a reliable way to wait until it is ready
-        this._modeReady = new $.Deferred();
-        this.modeReady = this._modeReady.promise();


The docs above say you can get a Language right away by calling getLanguage() after defineLanguage(), but moving this into the done() handler makes that untrue.

This also makes the if (_languages[id]) check above unreliable -- as long as you can call defineLanguage() for two colliding languages before either one finishes loading, no warning.

So perhaps registering the id should be moved back here?

Fixed comment, added _pendingLanguages to track collisions.

peterflynn · 2013-03-01T07:26:19Z

src/language/LanguageManager.js

+
+        if (!isPlainText && (!mode || mode === "")) {
+            result.reject("Mode must be specified as a built-in CodeMirror mode or [\'\', 'text/plain']");
+        }



peterflynn · 2013-03-01T08:10:57Z

@jasonsanjose (CC @DennisKehrig) Done reviewing

…se special null string for plain text mode. Update unit tests.

jasonsanjose · 2013-03-01T18:37:07Z

@peterflynn ready to review

peterflynn · 2013-03-01T22:00:10Z

src/brackets.js

+            var extensionLoaderPromise = ExtensionLoader.init(params.get("extensions"));
+
+            // Load the initial project after extensions have loaded
+            extensionLoaderPromise.always(function () {


Of course thinking about it more, I realized there's a case this doesn't fix :-P The race condition still exists for any new modes added by extensions -- we begin project loading (including opening editors) after the extension itself has loaded but potentially before its mode has... So we should probably still leave #2962 open at a lower priority to try to come up with a more robust fix.

How about I close #2962 and file a new bug specifically for the extension use case?

peterflynn · 2013-03-01T22:10:50Z

Looks good to go -- merging.

I'd prefer we leave #2962 open though, since the main use case (calling from extensions) isn't fixed yet. But it seems ok to punt it to next sprint.

Continued Language API work

DennisKehrig · 2013-03-01T22:48:13Z

@peterflynn We could compromise by requiring some modes unconditionally again, the rest will then happen synchronously and therefore reliably. Still not airtight, though.

peterflynn · 2013-03-01T23:08:11Z

I believe Jason's last commit here guarantees that the only core mode subject to this bug is LESS. The main modes that would be vulnerable to the bug are those added by third-party extensions.

DennisKehrig and others added 7 commits February 27, 2013 14:02

Fix: language IDs were documented as foo_bar, but validated as foo.bar

3dceb96

Normalize file extensions by also removing leading dots if necessary

38a015c

Use CollectionUtils instead of $.each

37be3d9

Documentation cleanup

b582655

Call result.reject in a few places instead of throwing exceptions

Merge remote-tracking branch 'origin/master' into dk/language-api

2eaf956

Conflicts: test/spec/Editor-test.js

jasonsanjose reviewed Feb 27, 2013
View reviewed changes

peterflynn mentioned this pull request Feb 27, 2013

Continued Language API work #2979

Closed

peterflynn reviewed Feb 27, 2013
View reviewed changes

code review comments pull #2980

fc829e9

jasonsanjose mentioned this pull request Feb 28, 2013

Potential race condition with mode loading #2962

Closed

jasonsanjose added 3 commits February 28, 2013 09:50

Merge branch 'master' into jasonsanjose-dk/language-api

8a04dc1

fixed mode handling for text/plain and validation of mode

513e4e0

fix language unit tests

ce7fd4b

peterflynn reviewed Mar 1, 2013
View reviewed changes

code review comments part 1

53e4686

peterflynn reviewed Mar 1, 2013
View reviewed changes

code review comments part 2. Track _pendingLanguages while loading. U…

90cb5c5

…se special null string for plain text mode. Update unit tests.

jasonsanjose added 3 commits March 1, 2013 12:14

update language id validation

3a4ca20

update defineLanguage jsdoc

b42ac95

wait for LanguageManager to load before finish init

6fce307

peterflynn reviewed Mar 1, 2013
View reviewed changes

peterflynn added a commit that referenced this pull request Mar 1, 2013

Merge pull request #2980 from adobe/jasonsanjose-dk/language-api

4e44a07

Continued Language API work

peterflynn merged commit 4e44a07 into master Mar 1, 2013

peterflynn deleted the jasonsanjose-dk/language-api branch March 1, 2013 22:11

This was referenced Mar 2, 2013

Remaining code cleanups for language APIs #2968

Closed

Status bar no longer shows language #3025

Closed

core-ai-bot mentioned this pull request Aug 29, 2021

[CLOSED] Continued Language API work brackets-archive/bracketsIssues#2807

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Continued Language API work #2980

Continued Language API work #2980

jasonsanjose commented Feb 27, 2013

jasonsanjose Feb 27, 2013

peterflynn Feb 27, 2013

jasonsanjose Feb 27, 2013

DennisKehrig Feb 28, 2013

jasonsanjose Feb 28, 2013

peterflynn Feb 27, 2013

jasonsanjose Feb 27, 2013

DennisKehrig Feb 28, 2013

jasonsanjose commented Feb 28, 2013

jasonsanjose commented Feb 28, 2013

DennisKehrig commented Feb 28, 2013

jasonsanjose commented Feb 28, 2013

peterflynn Mar 1, 2013

jasonsanjose Mar 1, 2013

peterflynn commented Mar 1, 2013

peterflynn Mar 1, 2013

jasonsanjose Mar 1, 2013

peterflynn Mar 1, 2013

jasonsanjose Mar 1, 2013

peterflynn commented Mar 1, 2013

jasonsanjose commented Mar 1, 2013

peterflynn Mar 1, 2013

jasonsanjose Mar 1, 2013

peterflynn commented Mar 1, 2013

DennisKehrig commented Mar 1, 2013

peterflynn commented Mar 1, 2013

Continued Language API work #2980

Continued Language API work #2980

Conversation

jasonsanjose commented Feb 27, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonsanjose commented Feb 28, 2013

jasonsanjose commented Feb 28, 2013

DennisKehrig commented Feb 28, 2013

jasonsanjose commented Feb 28, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterflynn commented Mar 1, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterflynn commented Mar 1, 2013

jasonsanjose commented Mar 1, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterflynn commented Mar 1, 2013

DennisKehrig commented Mar 1, 2013

peterflynn commented Mar 1, 2013