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

VS Code Support #200

Merged
merged 17 commits into from
Oct 11, 2024
Merged

Conversation

mcecode
Copy link
Collaborator

@mcecode mcecode commented Oct 3, 2024

Hi, I finally got around to finishing things and making this PR. You may have already seen some of these changes since I saw that you starred my fork a few days ago, but here's a breakdown of what I've done and some notes about them.

Extension

  • The extension now expects harper-ls to be bundled with it.
  • Users can now manually restart harper-ls if they want.
  • When a user changes any setting contributed by the extension, harper-ls is now automatically restarted so those changes are reflected right away.
  • Added icon for the Marketplace.

Tests

I've added some integration tests, mostly to serve as a smoke test to ensure that the extension works with the current state of harper-ls. In the process, I removed unused scripts and dependencies in package.json and cleaned up the files in .vscode a bit.

justfile

  • Added test-vscode recipe.
  • Updated package-vscode recipe, mostly so it can be used by the workflow discussed below.
  • Opted to use test-vscode instead of package-vscode in both precommit, because it's a better gauge if the extension is working properly, and setup, because testing downloads extra resources that packaging doesn't.

Docs

  • Added an "Unreleased" section in CHANGELOG.md for the extension changes I've done.
  • Updated README.md for the Marketplace. I intentionally left the "Installation" section empty for when you're ready to release or have released the extension as people might be misled when they get an error trying to install elijah-potter.harper and get an error.
  • Added development-guide.md to make it easier for those who want to contribute.

Publishing Workflow

I created a workflow to assist in publishing the extension, basing on build_harper_ls.yml, it compiles harper-ls for every platform currently supported, packages it with the extension, then publishes the extension for each platform. It can be triggered by pushing a tag prefixed with harper-vscode or manually in the GitHub Actions UI. For the workflow to work, you need to create the elijah-potter publisher, get a Personal Access Token (PAT), and create a secret named VSCE_PAT set to the value of the PAT you previously obtained.

My idea with this is that the extension would have the same version as harper-ls so it's easy for users (and us if they file an issue) to know that extension version X is also harper-ls version X. So when publishing with this workflow, the main manual work that needs to be done before running it is to update the version in package.json and maybe change the "Unreleased" section in CHANGELOG.md, if there is any, to the new version. That said, I'm open to other ideas, maybe you prefer a different way of publishing the extension or you want to reduce the manual steps or maybe you want to test the extension first before it's packaged to be sure it works.

Note, you may notice that I added "endOfLine": "auto" to .prettierrc and that's because Prettier throws a fit in Windows without this. Here's an example of a run where everything is okay with macOS and Linux but not Windows.

Closes #116

@grantlemons grantlemons added the enhancement New feature or request label Oct 3, 2024
Copy link
Collaborator

@elijah-potter elijah-potter left a comment

Choose a reason for hiding this comment

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

Overall, I think this is really great work. I'd like to hold back on the publish workflow (see my comments), but otherwise I think it's very close to being ready to merge.

Once we do merge it, I'll add you as a collaborator to the repo.

.github/workflows/publish_vscode_plugin.yml Outdated Show resolved Hide resolved
.github/workflows/publish_vscode_plugin.yml Outdated Show resolved Hide resolved
justfile Show resolved Hide resolved
packages/vscode-plugin/.vscode/settings.json Show resolved Hide resolved
packages/vscode-plugin/CHANGELOG.md Show resolved Hide resolved
| ------------------------------- | ------------------------------- | ------------------- |
| Harper: Restart Language Server | `harper.languageserver.restart` | Restart `harper-ls` |

### Settings
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it necessary to include linter descriptions in the README? I would imagine they should be self-explanatory enough to stand alone in the settings page. I'm not a huge fan of the duplication with package.json.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I understand. I'd also prefer not to duplicate them, but I just thought it might help people who find out about Harper from the VS Code Marketplace first to get an overview of what exactly it checks for. However, I can remove it if you think it is unnecessary.

packages/vscode-plugin/development-guide.md Show resolved Hide resolved
packages/vscode-plugin/package.json Show resolved Hide resolved
@elijah-potter
Copy link
Collaborator

I would like to test this out on a couple systems other than my own. @lukasmwerner can you confirm that this works on ARM Mac? Make sure you remove your existing harper-ls before testing.

@lukasmwerner
Copy link
Contributor

lukasmwerner commented Oct 4, 2024

Confirmed working on M1 running Sonoma 14.0! Loads the bundled harper-ls build perfectly. No issues, even after adding my local install of harper-ls back, very impressive!

Once I have time later today I'll test on my Alpine Linux install.

@elijah-potter
Copy link
Collaborator

I've done some testing and it looks like our current CI setup for Windows is broken. See #208. I'd like to get this fixed before merging.

@elijah-potter
Copy link
Collaborator

Sorry this has takes so long to get through. I'm going to merge it and cut a release as soon as possible. Thank you so much for all you work.

@elijah-potter elijah-potter merged commit cc58412 into Automattic:master Oct 11, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

VS Code Support
5 participants