search

brackets-archive / bracketsIssues

Archive of issues in brackets.
0 stars 0 forks source link

[CLOSED] Remaining code cleanups for language APIs #2800

Open core-ai-bot opened 3 years ago

core-ai-bot commented 3 years ago

Issue by peterflynn Wednesday Feb 27, 2013 at 00:58 GMT Originally opened as https://github.com/adobe/brackets/issues/2968

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)

  1. 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).
  2. 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.
  3. 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.
  4. I think Language._setMode() can just use Array.isArray() instead of $.isArray()
  5. The way LanguageManager sets its exports at the end doesn't match our usual format
  6. Docs on the defineMIME("text/x-brackets-html"... call could use clarification (see this comment)
  7. 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...
  8. JSDocs in Editor still reference Languages instead of LanguageManager
  9. Nit: Seems like _fileExtensionsToLanguageMap should be named _fileExtensionToLanguageMap (without the "s") -- more consistent with how we name other maps (the key is not plural)
core-ai-bot commented 3 years ago

Comment by peterflynn Wednesday Feb 27, 2013 at 01:14 GMT

Oh, one more:

core-ai-bot commented 3 years ago

Comment by peterflynn Wednesday Feb 27, 2013 at 01:38 GMT

One more...

core-ai-bot commented 3 years ago

Comment by peterflynn Wednesday Feb 27, 2013 at 02:36 GMT

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.

core-ai-bot commented 3 years ago

Comment by TomMalbran Wednesday Feb 27, 2013 at 02:46 GMT

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

core-ai-bot commented 3 years ago

Comment by DennisKehrig Wednesday Feb 27, 2013 at 13:24 GMT

Jason fixed the exports issue in 81aa8961a4cab50158259ae57ce8b0d254c946ab

core-ai-bot commented 3 years ago

Comment by DennisKehrig Wednesday Feb 27, 2013 at 18:29 GMT

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.

core-ai-bot commented 3 years ago

Comment by DennisKehrig Thursday Feb 28, 2013 at 09:20 GMT

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

I suppose I'm just traumatized by PHP:

With a-z, we have:

With a-z, separated by _ we have:

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

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.

core-ai-bot commented 3 years ago

Comment by peterflynn Friday Mar 01, 2013 at 08:18 GMT

@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).

core-ai-bot commented 3 years ago

Comment by jasonsanjose Friday Mar 01, 2013 at 18:16 GMT

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

core-ai-bot commented 3 years ago

Comment by pthiess Friday Mar 01, 2013 at 19:23 GMT

reviewed - medium priority

core-ai-bot commented 3 years ago

Comment by peterflynn Friday Mar 01, 2013 at 19:56 GMT

@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.

core-ai-bot commented 3 years ago

Comment by DennisKehrig Friday Mar 01, 2013 at 20:05 GMT

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.

core-ai-bot commented 3 years ago

Comment by peterflynn Saturday Mar 02, 2013 at 01:53 GMT

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