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

Optimize scripts/clone_helixlsp_config.py for performance #102

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

YashRL
Copy link

@YashRL YashRL commented Mar 13, 2024

Here are the optimizations I made to the code:

  1. Reduced Redundancy: Instead of repeatedly accessing lsp.get("args", []), I assigned it to a variable cmd and reused it where needed.

  2. Simplified Conditionals: I simplified the conditional checks for the presence of "args" and "settings" keys in the LSP dictionaries by removing unnecessary if conditions.

  3. Optimized Loops: I avoided repeatedly accessing the same keys in dictionaries by storing them in variables (lsp and lang).

  4. Removed Unnecessary Operations: I removed redundant operations such as deleting the "args" key if it exists in the LSP dictionaries.

  5. Improved Readability: I made the code more readable by breaking long lines and using descriptive variable names.

These optimizations aim to improve the code's efficiency and readability without changing its behavior or adding new features.

Please check and tell me if I make any mistakes. I'm new to the world of open source.

Copy link
Owner

@joouha joouha left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time to make this PR.

I've added a few comments on your changes.

If you want to get this PR merged, please revert all changes to debug_vt100_input.py, and revert the changes to line 14 of clone_helix_lsp_config.py.

@@ -11,7 +11,8 @@

for lsp in lsps.values():
# Margs command and args
lsp["cmd"] = [lsp["command"], *lsp.get("args", [])]
cmd = [lsp["command"], *lsp.get("args", [])]
lsp["cmd"] = cmd
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think there's any need for this: cmd is not actually reused anywhere, so this change is just adding a variable assignment unnecessarily.


def callback(key_press: KeyPress) -> None:
Copy link
Owner

Choose a reason for hiding this comment

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

I have chosen to use type annotations in this code-base. because I find it helps me catch errors and bugs.

Why do you think they should be removed from this script in particular?

lsp = lsps.get(name, {})
if "languages" not in lsp:
lsp["languages"] = set()
lsp["languages"].add(lang["name"])
Copy link
Owner

Choose a reason for hiding this comment

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

This change I'm happy to keep

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.

2 participants