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

Speed up formatting #49

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

vfonic
Copy link

@vfonic vfonic commented Oct 6, 2023

Thanks for the great extension!

I noticed that the formatting is quite slow and it's not practical to wait 3-5 seconds for the formatting to complete.
I figured out that the majority of the time is spent on the start of htmlbeautifier command.
The actual formatting is quite fast, after the booting is done.

In order to speed up the formatting, I modified the extension so that it always has an htmlbeautifier process started and waiting for input.
When the format is called, the running htmlbeautifier process is passed the input and, after the output is returned and process exits, a new htmlbeautifier process is started.

@aliariff
Copy link
Owner

aliariff commented Oct 7, 2023

Hi @vfonic

Thank you for submitting the PR! I've reviewed the changes and have a couple of comments:

  • Error Handling for Initialization: It seems that the initialization process doesn't throw an error when the htmlbeautifier binary file is missing. We might want to handle this to provide a more informative message to the user.

  • Configuration Changes: I've noticed that while the process is reinitialized after each formatting operation (this.htmlbeautifier = this.initializeBeautifier();), it's still set up based on the configuration at that time. This means that if there's a configuration change after that, the subsequent format operation will still use the old configuration, and only the one after that would use the updated configuration. We might want to revisit this behavior to ensure that changes in the configuration are immediately reflected in the very next format operation. One idea to solve this is we could actively listen for any configuration changes related to the extension and then reinitialize the process. VS Code provides an event onDidChangeConfiguration which can be used for this purpose.

Let's address these points to enhance the robustness and flexibility of the solution. Again, thank you for your efforts, and looking forward to the updates!

@vfonic
Copy link
Author

vfonic commented Oct 20, 2023

Hi @aliariff!

I just ran the tests to check what happens with the current 0.4.1 code when htmlbeautifier doesn't exist.
It seems like both 0.4.1 and my changes fail with "htmlbeautifier executable not found" burried deep down in the error message. Perhaps this should be more prominent?

0.4.1 my changes
Screenshot 2023-10-20 at 19 30 04 Screenshot 2023-10-20 at 19 31 16

0.4.1 seems to exit immediately, while my code gets stuck, it seems. This seems to be broken from before? Can you try uninstalling the gem and running tests to see what happens?

I've added the configuration change check, but this "gem is not installed" issue, I don't feel like it was resolved in the proper manner. Feel free to jump in if you have time and know how to fix this.

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.

None yet

2 participants