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(gh_actions_ls): Fix incorrect rootdir resolution #3573

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Crashdummyy
Copy link
Contributor

Regression of #3566

@Crashdummyy Crashdummyy requested a review from glepnir as a code owner January 21, 2025 13:36
@Crashdummyy
Copy link
Contributor Author

@justinmk
Sorry I made a big mistake in the function could you please have a look at this ?
Sorry for the trouble

@Crashdummyy Crashdummyy changed the title fix(gh_actions_ls): Fix incorrect root_dir resolution fix(gh_actions_ls): Fix incorrect rootdir resolution Jan 21, 2025
@Crashdummyy Crashdummyy force-pushed the master branch 2 times, most recently from 7438c59 to f4e0fff Compare January 21, 2025 21:10
@Crashdummyy Crashdummyy requested a review from glepnir January 22, 2025 10:46

for _, entry in ipairs(patterns) do
if filename:find(entry.pattern) then
local match = vim.fs.find(entry.root, { path = filename, upward = true })[1]
Copy link
Member

Choose a reason for hiding this comment

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

use vim.fs.dirname

Copy link
Contributor Author

@Crashdummyy Crashdummyy Jan 22, 2025

Choose a reason for hiding this comment

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

Like this ?

diff --git a/lua/lspconfig/configs/gh_actions_ls.lua b/lua/lspconfig/configs/gh_actions_ls.lua
index 9ea39b85..ebd48f60 100644
--- a/lua/lspconfig/configs/gh_actions_ls.lua
+++ b/lua/lspconfig/configs/gh_actions_ls.lua
@@ -15,7 +15,8 @@ return {
 
       for _, entry in ipairs(patterns) do
         if filename:find(entry.pattern) then
-          local match = vim.fs.find(entry.root, { path = filename, upward = true })[1]
+          local dir = vim.fs.dirname(filename)
+          local match = vim.fs.find(entry.root, { path = dir, upward = true })[1]
           if match then
             return match
           end

What do you want me to replace ?

EDIT:
Or dont you like any filename: ?
I could o something like this

      local dirs_to_check = {
        '.github/workflows',
        '.forgejo/workflows',
        '.gitea/workflows',
      }
      local dir = vim.fs.dirname(filename)
      for _, subdir in ipairs(dirs_to_check) do
        local match = vim.fs.find(subdir, { path = dir, upward = true })[1]
        if match and vim.fn.isdirectory(match) == 1 and vim.fs.dirname(filename) == match then
            return match
        end
      end

@dundargoc
Copy link
Member

What exactly is the regression? When I test out gh_actions on master it seems to work for me.

@Crashdummyy
Copy link
Contributor Author

What exactly is the regression? When I test out gh_actions on master it seems to work for me.

oh really ?
In my case it doesnt load at all.

this branch:
image

master:
image

Maybe due t nvim 0.11 or something like that I dont know.
This solution at least works accross all devices and combinations I tested

@dundargoc
Copy link
Member

What repo/file are you testing? I'd like to test it.

@Crashdummyy
Copy link
Contributor Author

What repo/file are you testing? I'd like to test it.

Take that one for instance

https://github.com/Crashdummyy/roslynLanguageServer.git

I cant open nvim .github/workflows/cleanup.yml
image

with this change
image

@dundargoc
Copy link
Member

OK, you're right, I can recreate the problem. Thanks. Let me take a look at the code.

@dundargoc
Copy link
Member

Does the following work? I tried it and autocompletion worked for me

 root_dir = require('lspconfig.util').root_pattern('.github/workflows', '.forgejo/workflows', '.gitea/workflows'),

@Crashdummyy
Copy link
Contributor Author

Crashdummyy commented Jan 22, 2025

Does the following work? I tried it and autocompletion worked for me

 root_dir = require('lspconfig.util').root_pattern('.github/workflows', '.forgejo/workflows', '.gitea/workflows'),

Tested that one as well.
First of all I thought I should not use root_pattern anymore.
Besides it only detects the existence of .github/workflows.

When I open any other random file it tries to attach as well.

EDIT:

# should be detected
.
├──.github
│    └── workflows
│     ├── CreateSnapshot.yml
│     ├── dotnet.yml
│     ├── loadtests.yml
│     ├── release.yml
│     ├── RestoreSnapshot.yml
│     └── tenantconfigs
│     └── action.yml
│
├── cmiLoadTestSchema.yml # should not be detected

@dundargoc
Copy link
Member

First of all I thought I should not use root_pattern anymore.

Yes, but then I said in #3573 (comment) that because of reasons we can still use root_pattern. The current approach of vim.fs.dirname(... is also fine.

@Crashdummyy
Copy link
Contributor Author

First of all I thought I should not use root_pattern anymore.

Yes, but then I said in #3573 (comment) that because of reasons we can still use root_pattern. The current approach of vim.fs.dirname(... is also fine.

Oh sorry I misread that.
It felt more like I am still allowed to use it but should prefer to avoid it.

Copy link
Member

@dundargoc dundargoc left a comment

Choose a reason for hiding this comment

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

OK, this works for me. There might be an easier way to achieve this but eh

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.

3 participants