-
Notifications
You must be signed in to change notification settings - Fork 0
Replication of original PR #46: Add ruby support via solargraph #4
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
base: replicated-pr-46-before
Are you sure you want to change the base?
Conversation
# Get the solargraph executable path | ||
try: | ||
result = subprocess.run([which_cmd, "solargraph"], check=True, capture_output=True, text=True, cwd=repository_root_path) | ||
executeable_path = result.stdout.strip() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🐛 Correctness Issue
Typo in variable name causes runtime error.
The variable 'executeable_path' has a typo and should be 'executable_path', which will cause inconsistency when referencing this variable later.
Current Code (Diff):
- executeable_path = result.stdout.strip()
+ executable_path = result.stdout.strip()
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
executeable_path = result.stdout.strip() | |
executable_path = result.stdout.strip() |
🔄 Dependencies Affected
multilspy/language_servers/solargraph/solargraph.py
Function: Solargraph.setup_runtime_dependencies
Issue: Variable name mismatch in subsequent references
Suggestion: Fix all references to use the correct variable name
Current Code (Diff):
- if not os.path.exists(executeable_path):
+ if not os.path.exists(executable_path):
- raise RuntimeError(f"Solargraph executable not found at {executeable_path}")
+ raise RuntimeError(f"Solargraph executable not found at {executable_path}")
- os.chmod(executeable_path, os.stat(executeable_path).st_mode | stat.S_IEXEC)
+ os.chmod(executable_path, os.stat(executable_path).st_mode | stat.S_IEXEC)
assert "registrations" in params | ||
for registration in params["registrations"]: | ||
if registration["method"] == "workspace/executeCommand": | ||
self.initialize_searcher_command_available.set() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🐛 Correctness Issue
Undefined class attribute usage.
The code attempts to use 'initialize_searcher_command_available' which is not defined in the class, causing a runtime AttributeError.
Current Code (Diff):
- self.initialize_searcher_command_available.set()
+ if hasattr(self, 'initialize_searcher_command_available'):
+ self.initialize_searcher_command_available.set()
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
self.initialize_searcher_command_available.set() | |
if hasattr(self, 'initialize_searcher_command_available'): | |
self.initialize_searcher_command_available.set() |
for registration in params["registrations"]: | ||
if registration["method"] == "workspace/executeCommand": | ||
self.initialize_searcher_command_available.set() | ||
self.resolve_main_method_available.set() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🐛 Correctness Issue
Undefined class attribute usage.
The code attempts to use 'resolve_main_method_available' which is not defined in the class, causing a runtime AttributeError.
Current Code (Diff):
- self.resolve_main_method_available.set()
+ if hasattr(self, 'resolve_main_method_available'):
+ self.resolve_main_method_available.set()
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
self.resolve_main_method_available.set() | |
if hasattr(self, 'resolve_main_method_available'): | |
self.resolve_main_method_available.set() |
# server -> client: {'jsonrpc': '2.0', 'method': 'language/status', 'params': {'type': 'ProjectStatus', 'message': 'OK'}} | ||
# Before proceeding? | ||
if params["type"] == "ServiceReady" and params["message"] == "ServiceReady": | ||
self.service_ready_event.set() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🐛 Correctness Issue
Undefined class attribute usage.
The code attempts to use 'service_ready_event' which is not defined in the class, causing a runtime AttributeError.
Current Code (Diff):
- self.service_ready_event.set()
+ if hasattr(self, 'service_ready_event'):
+ self.service_ready_event.set()
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
self.service_ready_event.set() | |
if hasattr(self, 'service_ready_event'): | |
self.service_ready_event.set() |
self.server_ready.set() | ||
await self.server_ready.wait() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🐛 Correctness Issue
Potential deadlock in event handling.
Setting and immediately waiting on the same event ('server_ready') creates a deadlock as the event is never reset between operations.
Current Code (Diff):
- self.server_ready.set()
- await self.server_ready.wait()
+ # self.server_ready.set()
+ # await self.server_ready.wait()
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
self.server_ready.set() | |
await self.server_ready.wait() | |
# self.server_ready.set() | |
# await self.server_ready.wait() |
PR Summary
Add Ruby Language Support via Solargraph Integration
Overview
This PR adds support for the Ruby programming language to the multilspy framework by integrating the Solargraph language server.
Change Types
Affected Modules
multilspy/multilspy_config.py
multilspy/language_server.py
multilspy/language_servers/solargraph/solargraph.py
multilspy/test_sync_multilspy_ruby.py
multilspy/test_multilspy_ruby.py
Notes for Reviewers