Add a new method, setSpellcheckerType, to pick spellchecker selection.
#115
Conversation
|
@rsese: I think this is ready for review. Would you be the best person to start? |
|
I'll bring this one up for you 👍 |
|
I'm asking for a sanity review and correctness to make sure I didn't miss anything. I do not need this pulled into atom/spell-check, I will do that in a second PR. |
There was a problem hiding this comment.
It looks like always-use-hunspell-spec.coffee is mostly a duplicate of spellchecker-spec.coffee. In situations like this, we've done some metaprogramming around the calls to describe and it in a loop to programmatically duplicate tests with a different configuration. Here's an example of running a subset of find and replace tests for two different values of the useRipgrep setting. Do you think you could do something like that here with setSpellcheckerType to avoid duplicating the test?
@nathansobo: My original reason for having the two versions is that I needed to test both normal operations and the "force hunspell" version at the same time. If I combine them together, I'm not sure the best way of running the same test file twice with two different options. Also the files are slightly different, so I was afraid that the if statements would get even more complex to maintain because you have two axes to control (windows/mac/linux verses default/force-hunspell). Suggestions? |
- Introduce a new API call, `setSpellcheckerType` which takes a constant to determine if a system checker should be used, if available (the default), to always use the system checker, or to always use Hunspell. - Duplicates the specs to have a force to Hunspell implementation.
|
@dmoonfire I went ahead and made the change I had in mind. I basically just loop over two boolean conditions surrounding the test. It adds a bit of complexity, but I think the savings in terms of duplication is worth it. What do you think? We'll see if the tests pass on CI, because I have a couple failing locally, possibly due to being on a different macOS version. |
spec/spellchecker-spec.coffee
Outdated
|
|
||
| @fixture.setDictionary('de_DE_frami', dictionaryDirectory) | ||
| for testAlwaysUseHunspell in [true, false] | ||
| fdescribe 'SpellChecker', -> |
There was a problem hiding this comment.
Should this be a fdescribe?
|
Published as |
This is in reference to #109. I'm tagging it as
work-in-progresssince I need the PRs to test across multiple platforms.