Conversation
Signed-off-by: Ben James <in@benjam.es>
|
Also see #15 |
|
Have you tested it with languages other than English? I did some testing of the approach you used, but since Aspell doesn't support multiple languages at the same time, when the user changes the language, words in English, even if correct, were shown as incorrect, which in most cases would be a problem, since the vast majority of projects have a mix of English and another language. |
| public function check(string $text): array | ||
| { | ||
| $misspellings = $this->filterWhitelistedWords(iterator_to_array($this->aspell->check($text))); | ||
| $misspellings = $this->filterWhitelistedWords(iterator_to_array($this->aspell->check($text, $this->config->languages))); |
There was a problem hiding this comment.
I stumbled across the fact that Aspell only supports one language:
https://github.com/tigitz/php-spellchecker/blob/dbbf94efc9761505a9fa9583537b5f9d1b53260d/src/Spellchecker/Aspell.php#L28
So if we let the user configure an array, it can cause an unexpected exception here!
There was a problem hiding this comment.
Yeah - it says one language, I have seen that. The config is currently an array as that is what is expected by Aspell.
Agreed - we could make our config take only a string and wrap that in an array, but I went with this for now.
There was a problem hiding this comment.
Yes, this was what I had in mind as well. Maybe that's something you could take a look at?
|
@julio-cavallari Yes, I ran into the same problem as well when experimenting with this feature. Maybe we need to check words in multiple languages and see if there was a spelling mistake in one of the languages. |
…ithin an array to Aspell Signed-off-by: Ben James <in@benjam.es>
f80adad to
1f74fae
Compare
c0nst4ntin
left a comment
There was a problem hiding this comment.
Looks good now! However as @julio-cavallari mentioned, changing the language to German in an English project causes some trouble.
Let's get this basic implementation merged and then we can see how to improve it
|
There is one part of this code that need to have in order for my tests to work., $misspellings = $this->filterWhitelistedWords(iterator_to_array($this->aspell->check($text, ['en_US'])));I've not done anything with it the language issue is obviously a large and complicated one, but if this can be included it would be a boost to me (I wouldn't have to keep ignoring it in Git), and I am not sure how other non-us locales go.. |
|
@benjam-es Is it maybe possible for you to resolve the conflicts? If they are minimal @nunomaduro can do it on stream. But preparing this would probably make things easier 👍🏼 |
|
@benjam-es Could you maybe also prepare a new test which sets the language and then tests a file in a different language? |
Signed-off-by: Ben James <in@benjam.es>
|
Merged main and fixed conflicts. Will look at a test |
|
Sorry, but at the moment I want to keep this simple. So, tabling this idea for now! |
|
@c0nst4ntin @nunomaduro I have added a test case - please try that text case without the PR to show the error |
What:
Description:
This enables us to specify the language that is used by Aspell. Where users are using another language, they can specify. Also, Aspell relies on the system locale if this is not set, so the current main branch fails for users without en_US locale
Related:
See issue #27