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.

Sign up for GitHub

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

ui: respect the silence as defined in user LSP settings #131

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

ui: respect the silence as defined in user LSP settings #131

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions st4_py38/lsp_utils/node_runtime.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,11 @@ def _resolve_node_runtime(
except Exception as ex:
log_lines.append(' * Binaries check failed: {}'.format(ex))
if selected_runtimes[0] != 'local':
lsp_cfg = sublime.load_settings('LSP.sublime-settings')
suppress_error_dialogs = cast(bool, lsp_cfg.get('suppress_error_dialogs') or False)
if suppress_error_dialogs:
log_lines.append(' * Download auto-skipped due to "suppress_error_dialogs" setting')
continue
if not sublime.ok_cancel_dialog(
NO_NODE_FOUND_MESSAGE.format(package_name=package_name), 'Download Node.js'):
log_lines.append(' * Download skipped')
Comment on lines +97 to 104
Copy link
Member

Choose a reason for hiding this comment

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

This is not strictly an error dialog but a dialog asking user to install local node instance. Doesn't seem right to me to tie it to that LSP setting.

There is already a way to prevent this dialog from appearing by setting "nodejs_runtime": ["system"] setting in lsp_utils settings.

Copy link
Author

Choose a reason for hiding this comment

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

Well, I did want to add non-error dialog setting for technical cases like this :), but that got rejected.

I'd still like to use the local system in general, just not be block-ui-notified about any issues, so the use case is exactly the same, and I can decide whether to just ignore these messages if it's a temp issue with system node, and I don't need a dupe, or install a local node if I don't plan to install a system one.

Although in this specific case there are no commands to manually install, right? So in addition to suppressing the dialog (which is desired) this prevents any local installation (which isn't desired and would be hard to debug)

Could we add a command to install node locally and notify that this command could be used in the message?

Copy link
Member

@rchl rchl Nov 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If you want to use the system node in general then why don't you just set the nodejs_runtime as I've suggested? You clearly don't want to install a "local" node instance or you would do it once and never care about this dialog anymore.

Your crusade against modal dialog might be going too far in this case. The modality of the dialog is quite useful in this case since it allows user to install node instance and everything works right out of the box then. Otherwise (with just a log message), the LSP would fail to initialize and then the user would have to at least re-open the file after installing local node manually.

Copy link
Author

Choose a reason for hiding this comment

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

In general, I want to be able to use either, not just the system one. And I don't install the local one when it's a dupe, but in cases when I didn't have a system one that's exactly what I did - installed the local version via this plugin.

Expand Down