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

Overhaul #726

Merged
merged 26 commits into from
Feb 20, 2025
Merged

Overhaul #726

merged 26 commits into from
Feb 20, 2025

Conversation

elijah-potter
Copy link
Collaborator

@elijah-potter elijah-potter commented Feb 19, 2025

There are a number of changes in here, all aimed towards refactoring Harper's core engine to be more faster, simpler, and more extensible.

The biggest change (so far) was to rework the LintGroup type to be runtime-extensible. This significantly reduces the number of types the Rust compiler must churn through and therefore significantly improves compile times.

Additionally, I've changed the naming conventions of the Harper lint setting keys. If you configure harper-ls through lspconfig, you may need change your settings to match the new schema (which will be outlined in our Neovim configuration documentation). In VSCode or Obsidian, you may have to reconfigure your setup. See #667 for why we are doing this.

I will also be updating the Rust version and removing a ton of boilerplate in Patterns.

The goal is to do as many wide-reaching changes in one PR, in order to pay back tech debt in a way that avoids merge conflicts.

@elijah-potter elijah-potter added rust Pull requests that update Rust code javascript Pull requests that update Javascript code harper-core linting labels Feb 19, 2025
@mcecode
Copy link
Collaborator

mcecode commented Feb 20, 2025

So are we settling on harper-ls.camelCase.PascalCase as the pattern for all of our configs now? If so, then the following also need to be updated for consistency's sake:

  • harper-ls.codeActions.forceStable > harper-ls.codeActions.ForceStable
  • harper-ls.markdown.ignore_link_title > harper-ls.markdown.IgnoreLinkTitle

As for the precommit workflow failing, it seems the order that diagnostics are sent has changed, maybe due to the changes in harper-core. So updating the second test from

	it('gives correct diagnostics', () => {
		compareActualVsExpectedDiagnostics(
			getActualDiagnostics(markdownUri),
			createExpectedDiagnostics(
				{
					message: 'Did you mean to repeat this word?',
					range: createRange(2, 39, 2, 48)
				},
				{
					message: 'Did you mean to spell “errorz” this way?',
					range: createRange(2, 26, 2, 32)
				}
			)
		);
	});

to

	it('gives correct diagnostics', () => {
		compareActualVsExpectedDiagnostics(
			getActualDiagnostics(markdownUri),
			createExpectedDiagnostics(
				{
					message: 'Did you mean to spell “errorz” this way?',
					range: createRange(2, 26, 2, 32)
				},
				{
					message: 'Did you mean to repeat this word?',
					range: createRange(2, 39, 2, 48)
				}
			)
		);
	});

should fix the issue.


Lastly, packages/vscode-plugin/src/tests/fixtures/.vscode/settings.json should also be updated to reflect the new changes. So it should be updated from

{
	"files.associations": { "git-commit": "git-commit" },
	"harper-ls.linters.spell_check": true,
	"harper-ls.linters.repeated_words": true,
}

to

{
	"files.associations": { "git-commit": "git-commit" },
	"harper-ls.linters.SpellCheck": true,
	"harper-ls.linters.RepeatedWords": true,
}

@elijah-potter elijah-potter changed the title Performance Overhaul Overhaul Feb 20, 2025
@elijah-potter
Copy link
Collaborator Author

You're absolutely right @mcecode. I've updated the keys to match. Thanks for looking into why those tests were failing :).

@elijah-potter elijah-potter merged commit b95a5a1 into master Feb 20, 2025
11 checks passed
@elijah-potter elijah-potter deleted the perf-fixes branch February 20, 2025 21:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
harper-core javascript Pull requests that update Javascript code linting rust Pull requests that update Rust code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants