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

Fix texlab#GetProjectRoot #3610

Merged
merged 5 commits into from
Mar 12, 2021
Merged

Fix texlab#GetProjectRoot #3610

merged 5 commits into from
Mar 12, 2021

Conversation

ourigen
Copy link
Contributor

@ourigen ourigen commented Mar 2, 2021

This fixes the texlab#GetProjectRoot function based on the discussion in #2501 of using a .git folder as an indicator of a TeX project root. This should address the issue of the texlab LSP server not starting at all, as raised by #2790 and #2699

return ''
let l:project_root = ale#path#FindNearestDirectory(a:buffer, '.git')

return !empty(l:project_root) ? fnamemodify(l:project_root, ':h') : fnamemodify(a:buffer, ':h')
Copy link
Contributor

Choose a reason for hiding this comment

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

Returning the directory of the current buffer when no .git directory is found may cause ALE to spawn an instance of the language server for every tex file open. See the comment about this: #2501 (comment)

Here would be better to return an empty string if no root is found.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I personally didn't mind an LSP server starting for every tex file, but I agree it would not be preferred as the default. I will fix that now, and I'll also try to fix the tests after I understand them more.

@hsanson
Copy link
Contributor

hsanson commented Mar 7, 2021

please fix the failing test so I can merge this:

  Starting Vader: /testplugin/test/command_callback/test_texlab_command_callbacks.vader
    (1/5) [EXECUTE] The language string should be correct
    (2/5) [EXECUTE] The default executable path should be correct
    (3/5) [EXECUTE] The project root should be detected correctly
    (3/5) [EXECUTE] (X) '/testplugin/.git' should be equal to ''
    (4/5) [EXECUTE] The executable should be configurable
    (5/5) [EXECUTE] The options should be configurable
  Success/Total: 4/5

Previously, the function returned `../.git/`. We want the function to return the parent directory above that as the project root. This should help pass Vader tests.
@ourigen
Copy link
Contributor Author

ourigen commented Mar 8, 2021

@hsanson Tests should be fixed now. Thank you for the review!

Copy link
Contributor

@hsanson hsanson left a comment

Choose a reason for hiding this comment

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

Looking good. Thanks.

@hsanson hsanson merged commit 80a48d0 into dense-analysis:master Mar 12, 2021
jsit added a commit to jsit/ale that referenced this pull request Mar 30, 2021
* master: (214 commits)
  improve DMD handler (dense-analysis#3647)
  Add support for V: "v" (compiler) and "vfmt" fixer. (dense-analysis#3622)
  Add nixfmt as a Nix fixer. (dense-analysis#3651)
  Switch to using buildifier's -path option (dense-analysis#3640)
  Add support for `ptop` fixer (dense-analysis#3652)
  Add more parameters to the DMD linting command (dense-analysis#3639)
  dense-analysis#3633 - Move linter tests into test/linter
  Allow more time before PRs become stale
  Add support for clangd with CUDA (dense-analysis#3598)
  add support for svelte via svelteserver language server (dense-analysis#3644)
  dense-analysis#3633 - Put all dummy test files in test/test-files
  Add desktop-file-validate
  Fix a typo in a test filename
  issue 3033 (dense-analysis#3620)
  dense-analysis#3632 Add ale#util#MapMatches
  Fix ale#path#Dirname on Windows
  Disable blank issues and add a link to ask for help
  Check user systemd unit files with systemd-analyze
  Close dense-analysis#2102 - Add support for the Angular language server
  Fix texlab#GetProjectRoot (dense-analysis#3610)
  ...
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.

None yet

2 participants