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

Allow language inheritance (i.e. SVG extends XML) #3052

Closed
wants to merge 6 commits into from

Conversation

DennisKehrig
Copy link
Contributor

Fixes #3044
[now in master] Depends on / contains #3051 (Made Language.addFileExtension public)

@ghost ghost assigned peterflynn Mar 6, 2013
@DennisKehrig
Copy link
Contributor Author

Rebased onto #3051

@DennisKehrig
Copy link
Contributor Author

Rebased

@DennisKehrig
Copy link
Contributor Author

Includes #3164 so I could at least switch to my branch without git thinking I changed a translation file.

return LanguageManager.defineLanguage(definition.id, def);
if (def.lineComment) {
def.lineComment = def.lineComment.prefix;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This block shouldn't be needed anymore, since now def.lineComment is always defined as a string or an array, and not an object with a prefix key in it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, true. Thanks!

@DennisKehrig
Copy link
Contributor Author

Rebased, now without the Chinese translation issues.

@DennisKehrig
Copy link
Contributor Author

Will merge again once #3285 has landed, not before.

Conflicts:
	src/language/LanguageManager.js
@DennisKehrig
Copy link
Contributor Author

Merged master. Next merge only after being prompted.

@peterflynn
Copy link
Member

Sorry for the delay Dennis! We tagged this for review in the architecture meeting to get the ball rolling.

@gruehle
Copy link
Member

gruehle commented Apr 30, 2013

Reviewed in architecture meeting. Should add a new matches() method so callers of Editor.getLanguageForSelection() (and other similar methods) can check for actual or parent language matches. Existing code should be ported to use this new method.

@peterflynn
Copy link
Member

@DennisKehrig do you have cycles to add the matches() API proposed above (and port over existing usage of Language.getId())? We should be able to land it this sprint if so...

@peterflynn
Copy link
Member

Hmm, although I just realized another tricky area: some code uses language ids in a map, e.g. CodeHintManager. We'll need to update that code to traverse the language 'inheritance chain' and aggregate together all the map results rather than doing the simple single lookup it does today...

@WebsiteDeveloper
Copy link
Contributor

maybe there should be a utility function to do that.

@DennisKehrig
Copy link
Contributor Author

@peterflynn I will get to that later today!
I propose a LanguageManager.getEntryForLanguage(object, language) utility method, akin to what @WebsiteDeveloper suggested. Does that sound good?

@DennisKehrig
Copy link
Contributor Author

@peterflynn: I tried to require the LanguageManager in HintUtils.js, but I keep getting an error that brackets is not defined. Or even window, for that matter. Are we running this in a web worker somewhere?

@DennisKehrig
Copy link
Contributor Author

@peterflynn All done!

@peterflynn
Copy link
Member

@DennisKehrig Sorry for letting this languish for so long. After thinking about this for a few days, I believe we're not yet at a point where we need to introduce this concept into LanguageManager.

Many of the use cases for this in practice are simple enough to be handled via existing APIs -- e.g. an SVG code hint provider could register for "xml" generally but only respond in SVG files. In the past year we haven’t hit any cases that were really blocked by this.

If extensions start commonly providing much richer support for specific sub-languages in the future, it seems like we'd need to consider something like this. But we're not there yet, and in the meantime this adds a certain amount of complexity and makes dealing with language ids more error-prone (it's tempting to use them with ==, switch, map keys, etc. but that would no longer be safe -- you always need to use special utility functions instead). So we should hold off until the need becomes more clear.

Closing for now, but we will definitely revive this patch if needed down the road -- it's not forgotten :-) And thanks for all the effort & thought you put into this already!

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.

Extending support for existing languages
5 participants