Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

try to match at least 2 components of base scope with the map #2361

Merged
merged 10 commits into from
Nov 14, 2023

Conversation

rchl
Copy link
Member

@rchl rchl commented Nov 10, 2023

This is a candidate fix for sublimelsp/LSP-typescript#225.

If the base scope doesn't match exactly against our map then take out last components until maybe it does. Do it as long as there are two components or more.

This changed behavior can make more cases work without having to add explicit mappings but it can also "break" things. For example I had to add explicit text.html.vue mapping or otherwise it would fall-back to text.html mapping and receive a html language ID.

Let me know what you think. I think that this approach is potentially more universal than the previous one. And if it causes some scope to match incorrectly then we can always add more specific mapping to fix it.

plugin/core/types.py Outdated Show resolved Hide resolved
@predragnikolic
Copy link
Member

one test is failing:

======================================================================
FAIL: test_basescope2languageid (test_types.TestDocumentSelector)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/predragnikolic/.config/sublime-text/Packages/LSP/tests/test_types.py", line 166, in test_basescope2languageid
    self.assertEqual(basescope2languageid("source.js.react"), "javascriptreact")
AssertionError: 'javascript' != 'javascriptreact'
- javascript
+ javascriptreact
?           +++++

rchl and others added 2 commits November 12, 2023 10:54
Co-authored-by: Ilia <43654815+istudyatuni@users.noreply.github.com>
Comment on lines 150 to 175
def test_basescope2languageid(self) -> None:
self.assertEqual(basescope2languageid("source.js.vite"), "javascript")
self.assertEqual(basescope2languageid("source.c++"), "cpp")
self.assertEqual(basescope2languageid("source.coffee.gulpfile"), "coffeescript")
self.assertEqual(basescope2languageid("source.cs"), "csharp")
self.assertEqual(basescope2languageid("source.css.tailwind"), "css")
self.assertEqual(basescope2languageid("source.dosbatch"), "bat")
self.assertEqual(basescope2languageid("source.fixedform-fortran"), "fortran")
self.assertEqual(basescope2languageid("source.groovy.gradle"), "groovy")
self.assertEqual(basescope2languageid("source.groovy.jenkins"), "groovy")
self.assertEqual(basescope2languageid("source.js"), "javascript")
self.assertEqual(basescope2languageid("source.js.eslint"), "javascript")
self.assertEqual(basescope2languageid("source.js.gruntfile"), "javascript")
self.assertEqual(basescope2languageid("source.js.gulpfile"), "javascript")
self.assertEqual(basescope2languageid("source.js.postcss"), "javascript")
self.assertEqual(basescope2languageid("source.js.puglint"), "javascript")
self.assertEqual(basescope2languageid("source.js.react"), "javascriptreact")
self.assertEqual(basescope2languageid("source.js.stylelint"), "javascript")
self.assertEqual(basescope2languageid("sourcet.js.unittest"), "javascript")
self.assertEqual(basescope2languageid("source.js.webpack"), "javascript")
self.assertEqual(basescope2languageid("source.json-tmlanguage"), "jsonc")
self.assertEqual(basescope2languageid("source.json.babel"), "json")
self.assertEqual(basescope2languageid("source.json.bower"), "json")
self.assertEqual(basescope2languageid("source.json.composer"), "json")
self.assertEqual(basescope2languageid("source.json.eslint"), "json")
self.assertEqual(basescope2languageid("source.json.npm"), "json")
Copy link
Member Author

Choose a reason for hiding this comment

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

Having all those tests might be missing the point a bit. If we want to test that the function does proper matching then we should only need to test "unique" inputs from the point of the function. And should then likely mock the map so that it's clear from the test point of view what are all the inputs.

Unless we are testing the mappings and not the function itself which looks more like what we are doing here. But that doesn't do much for unittesting the code itself and it won't really help to catch the cases we don't account for now.

In any case, we can keep it but we can rewrite those with a lot less duplicated code.

Copy link
Member

Choose a reason for hiding this comment

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

I just wrote the test to test the mapping to guarantee that we don't miss some things.

But a agree that we can remove it, before we merge.

@jwortmann
Copy link
Member

I'm wondering if we shouldn't simply either...

  1. Add all the standard scopes for the language IDs from the table in the specs to this list, for example "source.python": "python". To be safe for any syntax alias from themes or file icon packages which might be introduced in the future.

or

  1. Fall back to use the second scope segment as language ID instead of the last segment. Then we would only need to add the exceptions in this list, for example text.tex.latex -> latex, but none of the "regular" scopes.

There are a few examples where probably no language server exists at the moment, but which might become problematic in the future. For example source.ini.editorconfig is used by both the INI and EditorConfig packages, but language ID would probably be ini. Or text.git.commit -> git-commit. And some (outdated or not) packages on Package Control might use some scope deviations which are probably missed at the moment, e.g. source.python.3.

@rchl
Copy link
Member Author

rchl commented Nov 12, 2023

2. Fall back to use the second scope segment as language ID instead of the last segment. Then we would only need to add the exceptions in this list, for example text.tex.latex -> latex, but none of the "regular" scopes.

Think I like that.

@rchl rchl merged commit 0422bc8 into main Nov 14, 2023
4 checks passed
@rchl rchl deleted the fix/scope-matching branch November 14, 2023 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants