feat(ruby-lsp): allow indexing vendored Rails engines via vendor_include_paths#1489
feat(ruby-lsp): allow indexing vendored Rails engines via vendor_include_paths#1489m3thom wants to merge 2 commits into
Conversation
|
CI failed with AI suggest to fix from pathlib import Path
+import pathspec
from solidlsp.language_servers.ruby_lsp import RubyLsp
from solidlsp.ls_config import Language
from solidlsp.settings import SolidLSPSettings
def _build_ruby_lsp(settings: SolidLSPSettings) -> RubyLsp:
language_server = RubyLsp.__new__(RubyLsp)
language_server._solidlsp_settings = settings
language_server.repository_root_path = ""
+ language_server._ignore_spec = pathspec.PathSpec.from_lines(pathspec.patterns.GitWildMatchPattern, [])
return language_server
But I'm not sure if it's a good solution. |
There was a problem hiding this comment.
Please simplify the code in this PR. It's too defensive, there are too many if and log statements. If a user misconfigures something let the LS handle it or crash. Never make if isinstance(list) or something like that. The functional extension here should need at most a few lines of change
|
|
||
| def is_ignored_path(self, relative_path: str, ignore_unsupported_files: bool = True) -> bool: | ||
| """Override to keep only configured vendor subtrees visible to Serena.""" | ||
| parts = pathlib.PurePosixPath(pathlib.Path(relative_path).as_posix()).parts |
There was a problem hiding this comment.
still too complicated at first glance. Consider normalizing the separator and then simply:
for vendor_path in self._custom_settings.get("vendor_include_paths", [])}:
if relative_path.startswith(vendor_path):
return False
return super().is_ignored_path(relative_path, ignore_unsupported_files)
If I'm not mistaken, that's all you need to do. Consider a similar pattern in the ignore pattern. We don't want to add any unnecessary, AI-generated complexity
|
Concerning
It is fine to generate changes with AI, we also do that all the time. But then it is the author's responsibility to thoroughly review the code, consider simplifications, consider how tests can be affected and so on. If that's not done, instead of helping the project, such PRs take away time from the maintainers and take longer to review and talk about than it would have been for us to make the change ourselves. |
|
@MischaPanch Thank you for your response and your time. I deeply apologize since at first I thought that this feature would require simple changes with AI help. But it turns out I complicated things. So, in my opinion, I would let you handle the changes. And again, I am deeply sorry for this. |
420a0ba to
016ccbe
Compare
Is your feature request related to a problem? Please describe.
Serena’s Ruby LSP integration currently excludes
**/vendor/**entirely. That works well for dependency directories, but it also hides application code for Rails projects that keep local engines undervendor/engines.This is especially relevant for older Rails applications. The Rails 4.1 engines guide shows vendored engines being added from
vendor/engines, for example:Source: https://guides.rubyonrails.org/v4.1/engines.html
In that setup, Serena ignores real application code inside the vendored engine, so symbols, definitions, and references from those engines are unavailable.
Describe the solution you'd like
Add a Ruby-specific
ls_specific_settings["ruby"].vendor_include_pathssetting that allows selected subtrees undervendor/to remain indexed while keeping the rest ofvendor/**ignored.Example:
With this change:
vendor/engines/**is visible to Serena and ruby-lspvendor/**paths remain ignoredvendor/engines/foo/vendor/**, remain ignoredThe implementation updates both Serena’s own path filtering and ruby-lsp’s
excludedPatterns, so the allowlist works end-to-end.Describe alternatives you've considered
vendor/**exclusion entirely. This would index too many dependency files and hurt performance.vendor/. That may work for newer Rails layouts, but it does not help older applications that already follow the vendored engine convention.For newer Rails applications, the current Rails engines guide commonly shows local engines outside
vendor/, for example:Source: https://guides.rubyonrails.org/engines.html#inside-an-engine
Those layouts are already handled without special configuration, since
engines/is not excluded by Ruby LSP.Additional context
This PR adds:
vendor_include_pathsvendor/**exclusionvendor/enginesvendor/**inside an included engine excludedAuthor note: this PR was created with help from AI. The author is not very familiar with the Python codebase, but after reading the changed code and running the relevant tests, the result looks correct.