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

Add a way to combine dictionary processing across packages #628

Closed
1 task done
jtiee opened this issue May 10, 2023 · 6 comments
Closed
1 task done

Add a way to combine dictionary processing across packages #628

jtiee opened this issue May 10, 2023 · 6 comments

Comments

@jtiee
Copy link

jtiee commented May 10, 2023

Check for existing issues

  • Completed

Describe the feature

I've started using Vale packages. Since a package can contain a Spelling.yml style, providing the Hunspell dictionaries alongside the style files in the package would be convenient.

When multiple packages define spelling styles, each style's spellchecks are evaluated independently. If a word like xyzzy is spelled correctly according to the dictionary in style B, spellchecking with style A flags the word as misspelled.

It would be great if all the dictionaries defined by all active styles could be processed together.

Related, it would be great if dicpath was relative to the Spelling.yml file or to the StylesPath instead of relative to where vale was executed. I recognize that this could be a breaking change, so perhaps a dictpath key could be introduced to handle the style-relative path, and using dicpath and dictpath together would be considered an error.

A style-relative dictpath would make it much easier to provide a style package that included its dictionaries and was agnostic to where vale is executed.

@jtiee jtiee changed the title Add a way to combine dictionary processing Add a way to combine dictionary processing across packages May 10, 2023
@jdkato
Copy link
Member

jdkato commented May 11, 2023

Related, it would be great if dicpath was relative to the Spelling.yml file or to the StylesPath instead of relative to where vale was executed.

This should already be the case. There are few different places that Vale searches:

// There are a few cases we need to consider:
paths := []string{
// 1. An absolute path (similar to $DICPATH)
s.Dicpath,
// 2. Relative to StylesPath
filepath.Join(cfg.StylesPath, s.Dicpath),
// 3. Relative to config file
filepath.Join(cfg.Root, s.Dicpath),
// 4. Relative to cwd
filepath.Join(cwd, s.Dicpath),
}

@jdkato
Copy link
Member

jdkato commented Sep 22, 2023

It would be great if all the dictionaries defined by all active styles could be processed together.

What would be your expectation for the case where a word doesn't appear in any of the dictionaries? In other words, there will be multiple rules (Style1.Rule, Style2.Rule, ...) reporting the same error.

@jtiee
Copy link
Author

jtiee commented Sep 22, 2023

In that scenario, it would be acceptable for every style that provides dictionaries to report an error. That's vastly superior to the current situation where a word defined in one style's dictionary but not in another style's dictionary causes an error.

Ideally, Vale would recognize that the error repeats across styles and reports a single error.

It might be helpful to report in the error all of the styles providing dictionaries so that the user can decide which dictionary to update. I recognize those would violate the way Vale currently reports errors, so I leave that up to you.

@jdkato jdkato mentioned this issue Sep 22, 2023
1 task
@jdkato
Copy link
Member

jdkato commented Sep 23, 2023

I've created a proposal (#688) for how I'd like to solve this: essentially, packages could contribute dictionaries to be consumed by Vale.Spelling instead of distinct spelling-based rules.

This handles both cases:

  1. An error will only be reported for words that aren't valid according to any dictionary.
  2. If an error is found, it will only be reported once (by Vale.Spelling).

Feel free to let me know what you think.

@jtiee
Copy link
Author

jtiee commented Sep 25, 2023

I like the proposal and agree that it should solve both problems.

@jdkato
Copy link
Member

jdkato commented Jan 10, 2024

This is out now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants