search

atom / node-spellchecker

SpellChecker Node Module
http://atom.github.io/node-spellchecker
MIT License
298 stars 106 forks source link

Add a new method, `setSpellcheckerType`, to pick spellchecker selection. #115

Closed dmoonfire closed 5 years ago

dmoonfire commented 5 years ago

This is in reference to https://github.com/atom/node-spellchecker/issues/109. I'm tagging it as work-in-progress since I need the PRs to test across multiple platforms.

dmoonfire commented 5 years ago

@rsese: I think this is ready for review. Would you be the best person to start?

rsese commented 5 years ago

I'll bring this one up for you :+1:

dmoonfire commented 5 years ago

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.

dmoonfire commented 5 years ago

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?

nathansobo commented 5 years ago

@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.

nathansobo commented 5 years ago

Published as spellchecker@3.7.0, thanks for your contribution! ❤️