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

[denols] Fix root_dir #1151

Closed
wants to merge 1 commit into from
Closed

[denols] Fix root_dir #1151

wants to merge 1 commit into from

Conversation

e-kwsm
Copy link
Contributor

@e-kwsm e-kwsm commented Aug 16, 2021

At the moment denols may fail to start since root directory is not detected.

I referred to

root_dir = function(fname)
local root_files = {
'pyproject.toml',
'setup.py',
'setup.cfg',
'requirements.txt',
'Pipfile',
}
return util.root_pattern(unpack(root_files))(fname) or util.find_git_ancestor(fname) or util.path.dirname(fname)
end,

lua/lspconfig/denols.lua Outdated Show resolved Hide resolved
@mjlbach
Copy link
Contributor

mjlbach commented Aug 16, 2021

I'm not in favor of adding util.path.dirname as a fallback, given most deno projects should have a well defined structure. As otherwise noted, you added a redundant git check

@e-kwsm
Copy link
Contributor Author

e-kwsm commented Aug 17, 2021

Without this modification, deno doesn’t start when a user writes the very first code on a project or a snippet, which is a bit inconvenient.
The behaviour is consistent with pylsp.

@mjlbach
Copy link
Contributor

mjlbach commented Aug 17, 2021

That's not true, the server will not start if the user does not have a root directory which is a different matter.

@e-kwsm
Copy link
Contributor Author

e-kwsm commented Aug 24, 2021

I’m not sure why other servers such as pylsp and clangd are allowed to start under the cwd.

Is there something disadvantageous?

@mjlbach
Copy link
Contributor

mjlbach commented Aug 24, 2021

Because the configs are community contributed, and I wasn't historically as strict about undefined behavior as I'm being now.

I'll be removing those fallbacks shortly.

@oblitum

This comment has been minimized.

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.

4 participants