Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

Allow non-us-english languages, respect system language default, test cleanup #23

Closed
wants to merge 9 commits into from
Closed

Allow non-us-english languages, respect system language default, test cleanup #23

wants to merge 9 commits into from

Conversation

hollandjg
Copy link

Modifications here were tested on Mac; I don't have another platform on which to test.

Problems addressed:
#15 -- by removing the hard-coded setting of the default language.
Cleaned up spec file to remove the redundant test of the setDictionary
Added a more user friendly setLanguage() which calls setDictionary() but doesn't require any knowledge of the dictionary path (could be useful for solving atom/spell-check#21); also allows more robust testing of behaviour on platforms with non-english default languages.
Rearranged spellchecker file so that functions can be read in order from top to bottom.

Testing required:
Do other platforms have the same behaviour on instantiation of the spellchecker -- namely they set a language default based on the system?

@anaisbetts
Copy link
Contributor

Rearranged spellchecker file so that functions can be read in order from top to bottom.

If you do things like that, can you do them in a separate PR? This makes the PR harder to understand

@anaisbetts
Copy link
Contributor

Do other platforms have the same behaviour on instantiation of the spellchecker -- namely they set a language default based on the system?

No, neither Hunspell nor Win8 do this

@anaisbetts
Copy link
Contributor

Added a more user friendly setLanguage() which calls setDictionary() but doesn't require any knowledge of the dictionary path (could be useful for solving atom/spell-check#21); also allows more robust testing of behaviour on platforms with non-english default languages.

So, while I think the idea of this PR is 💎, the hard-coded default setting is really the "compatibility API" - the API that people should be moving to is instantiating the Spellchecker class, which does support multiple languages properly and has no notion of a "default" language - see #6 for the details

@hollandjg
Copy link
Author

Thanks for the feedback! I'll produce a new PR with just the language support changes.

@maxbrunsfeld maxbrunsfeld force-pushed the master branch 8 times, most recently from 3d3dacc to 6bb8b4c Compare January 6, 2016 19:34
@havenchyk
Copy link

Hey @hollandjg, I guess we still need to add dictionaries for every language we would like to check, isn't it?

@hollandjg hollandjg closed this May 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants