Remaining code cleanups for language APIs

[peterflynn]

This is a continuation of the code review in #2844. The branch was landed before some smaller issues were addressed to give it additional bake time.

Remaining issues:

1. @jasonsanjose and both felt that mode shouldn't be optional when calling defineLanguage(). It seems virtually guaranteed to be a bug in the extension, so best to fail loudly instead of silently. To handle the rare case where a developer wants to add limited language support before code coloring is available, the developer could just explicitly specify "text/plain" as the mode. (See this comment and to some degree this one)
2. Language's constructor allows id to contain "." but not "_", the opposite of the docs. Though I actually like "." better since it's more consistent with our other id naming conventions, and I don't really see the need for explicit restrictions anyway... (See this comment).
3. Language._addFileExtension() may want to check for a "." in the string since it'd be an easy mistake to make. Or we could handle both like getLanguageForFileExtension() does.
4. The iteration over _defaultLanguagesJSON should use CollectionUtils.forEach() instead of $.each() -- we've been moving away from $.each() because it's buggy if arbitrary string keys are possible.
5. I think Language._setMode() can just use Array.isArray() instead of $.isArray()
6. The way LanguageManager sets its exports at the end doesn't match our usual format
7. Docs on the defineMIME("text/x-brackets-html"... call could use clarification (see this comment)
8. Language doesn't have JSDocs (or a prototype declaration) for these fields: mode, blockComment, lineComment, _fileExtensions, _modeToLanguageMap, _modeReady * Should blockComment, lineComment be private though? They have a setter you're supposed to use...
9. JSDocs in Editor still reference Languages instead of LanguageManager
10. Nit: Seems like _fileExtensionsToLanguageMap should be named _fileExtensionToLanguageMap (without the "s") -- more consistent with how we name other maps (the key is not plural)

[peterflynn]

Oh, one more:

• It feels odd that FileUtils causes an event to be dispatched on FileEntry. I'll accept that as a FOL with how hacked-in our rename support is, but we should probably at least document this event on FileEntry, in NativeFileSystem.

[peterflynn]

One more...

• Before this change we supported a "haxe" mode (due to community pull request) -- see EditorUtils. That got lost with this merge.

[peterflynn]

It looks like @TomMalbran has the "haxe" issue addressed in #2971, but I noticed one other seeming issue with Sprint 20 parity. We used to distinguish C and C++ modes but in the new languages.json they're all lumped together.

[TomMalbran]

@peterflynn I just pushed a commit on my pull request to address the C and C++ modes.

[DennisKehrig]

Jason fixed the exports issue in 81aa8961a4cab50158259ae57ce8b0d254c946ab

[DennisKehrig]

Sorry for messing up language support (Haxe, C/C++), those were very easy to miss since rebasing doesn't work very well when files get changed in master that are long gone in a new branch.

[DennisKehrig]

This is my final comment about being strict about language IDs.

I suppose I'm just traumatized by PHP:

• strpos
• readline
• imagesx
• file_get_contents
• set_exception_handler
• shell_exec
• FrenchToGD
• hw_Deleteobject
• PDF_begin_document

With a-z, we have:

• cpp
• cplusplus

With a-z, separated by _ we have:

• cpp
• c_p_p
• cplusplus
• c_plusplus
• c_plus_plus

With a-z, A-Z and _ we have:

• cpp
• Cpp
• CPP
• c_p_p
• C_p_p
• C_P_P
• Cplusplus
• CPlusPlus
• c_plus_plus
• C_plus_plus
• C_Plus_Plus

I'm sure I forgot some.

What's the benefit of allowing that? I am not sure. I'd like to see a use case. I remember Peter talking about package style names. For languages, though? Will we really have cpp, dk_cpp, pf_cpp, jsj_cpp? All with the same file extensions, thus conflicting anyway? I think it's perfectly fine to have a choice for what extension to install in order to support C++, but if I depend on that language in my extension, I don't want want to have to do something like that: var cpp = LanguageManger.getLanguage("cpp") || LanguageManger.getLanguage("dk_cpp") || LanguageManger.getLanguage("pf_cpp"), having to update my extension when somebody else decides to provide another alternative.

But I suppose this is not what we're worried about. We're worried about two different languages ending up using the same ID, right? But that would mean that if I were to introduce SASS support, I would define it with the ID "denniskehrig_sass" to make sure it's unique. And then Joe Developer wants to provide SASS support, doesn't know about my extension, and defines it as "joedeveloper_sass". Awesome, now if Jane Foo wants to write an extension that requires SASS support she has to be able to specify that she either needs the language denniskehrig_sass OR joedeveloper_sass. Or it'll just work with denniskehrig_sass and someone who has joedeveloper_sass installed just can't use her extension. Or the extension will install fine because both SASS extensions are installed, but her extension won't do anything because joedeveloper_sass snatches all SASS files, so that denniskehrig_sass is never reported.

I think we want our extension repository to list which names currently exist under which ID so people who write a language extension can stick to an existing ID or come up with a new one if there's a conflict with a different language. And then the need for package style IDs is pretty much gone anyway. Command IDs are different in nature, they are usually NOT intended to be used by somebody else. Language IDs however ARE. Still, had we been more strict about command IDs, writing that extension that lists what keyboard shortcuts are defined by which extensions might have been easier, too.

I'll leave it at that. It's a democracy after all.

[peterflynn]

@DennisKehrig thanks for the detailed explanation. I missed your reply in 2844, so glad I caught this one. These seem like reasonable concerns, so I'm ok with leaving the restrictions as they stand (now that they match the docs).

[jasonsanjose]

Ok, I'll change the validation to be lowercase a-z only.

[pthiess]

reviewed - medium priority

[peterflynn]

@jasonsanjose Hah, ok... so we're going back to lowercase-only now? Can we at least change the documentation & validation error string to say so this time? :-P

Also, if we're going to touch this code again can we add numbers to the list of allowed values? Otherwise you can't have languages like "css3" or "c_99" which seem like both reasonable and useful language ids.

[DennisKehrig]

I agree with adding numbers, you mention reasonable examples and numbers are unproblematic, I think. I'm glad that @jasonsanjose changed it back to a-z, obviously, though I understood @peterflynn's comment differently. :) But I think it makes sense to wait until somebody complains - we can still change it then, and prevent an unnecessary mess in the meantime.

[peterflynn]

Closing since I think everything here has been addressed now that #2980 is in.