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

feat(vscode-plugin): config updates #754

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

Conversation

mcecode
Copy link
Collaborator

@mcecode mcecode commented Feb 23, 2025

Issues

Related to #667 since that's what brought up talks about look and feel.

Description

  • Only send Harper specific settings to harper-ls. Prior to this change, VS Code sent up the whole user configuration by default. Aside from being the sane thing to do, this helps when debugging messages between VS Code and harper-ls since there's less cruft in the logs.
  • Change setting key prefixes from harper-ls to harper. This change only applies to VS Code, the prefixes are changed to harper-ls before being sent to harper-ls. It's primarily for aesthetic purposes in the Settings UI.

Demo

Before, with the awkward Harper-ls:

harper-pr-1

After, with just the title:

harper-pr-2

How Has This Been Tested?

  • Manually in the extension host
  • just test-vscode

Checklist

  • I have performed a self-review of my own code
  • I have added tests to cover my changes

@mcecode
Copy link
Collaborator Author

mcecode commented Feb 23, 2025

On a side note, while I was making this PR I noticed that we pull the config every time we update a document:

self.pull_config().await;

I'm just wondering if that's really necessary since nowhere in update_document do we actually check and use any pulled updates. Additionally, any updates would already get picked up by did_change_configuration. It's not really important performance-wise since these exchanges take single-digit milliseconds, but it just seems like a waste to me if it's not really needed. Is it perhaps because there are editors that don't support did_change_configuration?

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.

1 participant